From 1a23d164dbe643256e0f43ae4459b731b37e39cc Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 07:43:06 +0000 Subject: [PATCH 1/2] fix(metadata): DatabaseLoader read failures are outages, not "nothing declared" (#5108) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five read methods on `DatabaseLoader` used to `catch {}` any storage error into their own empty value — `load` → `null`, `loadMany` → `[]`, `exists` → `false`, `stat` → `null`, `list` → `[]`. An unreachable `sys_metadata` was therefore byte-identical to "this environment declares nothing of that type", and because the exception was erased *inside the loader*, none of `MetadataManager`'s degradation branches could fire either: they received a successful empty read, not a failure. Nowhere on the chain was there a line saying the read failed. Discriminate by error TYPE instead (rule from #4632; same shape as #4728 and #4825 in this very file, reusing the existing `isMissingTableError`): - the ONE benign reason is `sys_metadata` not being provisioned yet — there are then genuinely no rows, so a first boot still answers empty, silently, and without caching the answer; - every other reason (connection drop, timeout, insufficient privileges, query error) means the rows may well be there and simply were not seen, so the driver error is rethrown UNCHANGED and the caller owns the consequence. Classification is conservative: an unrecognised error is not benign. Three mechanisms one layer up become honest as a result: - `MetadataManager.list()` actually enters its degradation branch, now at `error` (AGENTS.md → "Degradation log levels": the system looks normal while the set it gates on is short), naming the consequence and the fix, said once per outage and un-said on recovery. `list()` still serves what the reachable loaders hold — that best-effort posture is deliberate. - `MetadataManager.loadDiagnosed()` (ADR-0110 D3) can finally report `degraded`/`errors` for `DatabaseLoader` rather than reporting an outage as a miss. - `listForIndex()` / `matchEndpoint` (#5089) — whose contract requires an unreadable store to THROW rather than masquerade as a miss, because a miss becomes a 404 — now holds against the real datasource-backed loader and not just the memory/remote ones. Its stale ⚠️ note pointing at this issue is replaced. Behaviour change: `MetadataManager.exists()` and `listNames()` carry no `try/catch`, so a storage outage now propagates out of them instead of being answered as "does not exist" / "empty list". That is the intended posture — an availability failure is not an absence. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 --- ...tabase-loader-read-outage-is-not-a-miss.md | 37 +++ .../src/loaders/database-loader.test.ts | 274 ++++++++++++++++-- .../metadata/src/loaders/database-loader.ts | 75 ++++- packages/metadata/src/metadata-manager.ts | 79 ++++- 4 files changed, 435 insertions(+), 30 deletions(-) create mode 100644 .changeset/database-loader-read-outage-is-not-a-miss.md diff --git a/.changeset/database-loader-read-outage-is-not-a-miss.md b/.changeset/database-loader-read-outage-is-not-a-miss.md new file mode 100644 index 0000000000..e50f49f693 --- /dev/null +++ b/.changeset/database-loader-read-outage-is-not-a-miss.md @@ -0,0 +1,37 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): `DatabaseLoader` 的读故障不再被吞成「什么都没声明」(#5108) + +`DatabaseLoader` 的五个读方法此前都把**任何**存储异常 `catch {}` 成各自的空值 —— +`load` → `null`、`loadMany` → `[]`、`exists` → `false`、`stat` → `null`、 +`list` → `[]`。于是 `sys_metadata` 所在库不可达时,`loadMany('permission')` 与 +「这个环境一条 permission 都没声明」返回**完全一样的值**,而且异常是在 loader 内部 +就被抹掉的:`MetadataManager` 那几个 `try/catch` 降级分支拿到的是一次「成功的空读」, +根本不会触发,整条链上没有任何一处会说出「读失败了」。 + +现在按**错误类型**判决(#4632 立的规矩,#4728 / #4825 已经在同一个文件里用过两次的 +形状,判据复用现成的 `isMissingTableError`): + +- 唯一良性的失败原因是 `sys_metadata` 尚未 provisioned —— 那时确实没有行, + 「什么都没声明」就是事实,首次启动照旧返回空值、不报错、不缓存; +- 其余全部原因(连接断开、超时、权限不足、查询出错)意味着行还在、只是这次没读到, + 一律把驱动原始异常**原样抛出**,由调用方决定降级姿态。判据保守:无法正面识别为 + 「表不存在」的错误一律当作真故障。 + +由此上层三个已有的机制第一次真的生效: + +- `MetadataManager.list()` 的降级分支会真的进,并且**升级到 `error`** + (AGENTS.md「Degradation log levels」:系统看着正常、它声称掌握的清单其实是残缺的), + 日志写明后果与修法,每次故障只说一次、恢复时再说一次;`list()` 仍然尽力返回可读 + loader 的内容 —— 这个 best-effort 姿态是刻意保留的; +- `MetadataManager.loadDiagnosed()`(ADR-0110 D3)对 `DatabaseLoader` 终于能报出 + `degraded` / `errors`,而不是把 outage 报成 miss; +- `listForIndex()` / `matchEndpoint`(#5089)契约要求「读不到存储必须抛出,不得伪装成 + miss(miss 会变成 404)」—— 这条此前对 `MemoryLoader` / `RemoteLoader` 有效、对 + `DatabaseLoader` 无效,现在对真实的 datasource loader 也成立了。 + +**行为变化**:`MetadataManager.exists()` 与 `listNames()` 本来就没有 `try/catch`, +所以存储故障现在会从它们抛出,而不再静默答「不存在」/「空清单」。这正是本次修复要的 +姿态 —— 可用性故障不是一次「没有」。 diff --git a/packages/metadata/src/loaders/database-loader.test.ts b/packages/metadata/src/loaders/database-loader.test.ts index b64a6c3847..daea589793 100644 --- a/packages/metadata/src/loaders/database-loader.test.ts +++ b/packages/metadata/src/loaders/database-loader.test.ts @@ -6,14 +6,18 @@ import type { IDataDriver } from '@objectstack/spec/contracts'; import { MetadataManager } from '../metadata-manager'; import { MemoryLoader } from './memory-loader'; -// Suppress logger output during tests +// Suppress logger output during tests. Stable object (not a fresh one per +// `createLogger()` call) so the #5108 block can assert on what `list()` says +// when a loader cannot be read — the whole point of that fix is the log line. +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + vi.mock('@objectstack/core', () => ({ - createLogger: () => ({ - info: vi.fn(), - warn: vi.fn(), - error: vi.fn(), - debug: vi.fn(), - }), + createLogger: () => logger, })); /** @@ -378,46 +382,49 @@ describe('DatabaseLoader', () => { }); describe('error handling', () => { - it('should return null data on load failure', async () => { + // [#5108] These five used to assert the opposite — that a read failure is + // answered with the method's empty value. That behaviour WAS the defect: + // it made an unreachable `sys_metadata` byte-identical to "nothing of this + // type was ever declared". The read seam now discriminates by error type + // (see the dedicated #5108 block below for the benign half). + it('should rethrow a read failure from load — an outage is not a miss', async () => { const failingDriver = createMockDriver(); failingDriver.findOne = vi.fn().mockRejectedValue(new Error('DB error')); const failLoader = new DatabaseLoader({ driver: failingDriver }); - const result = await failLoader.load('object', 'account'); - expect(result.data).toBeNull(); + await expect(failLoader.load('object', 'account')).rejects.toThrow('DB error'); }); - it('should return empty array on loadMany failure', async () => { + it('should rethrow a read failure from loadMany', async () => { const failingDriver = createMockDriver(); failingDriver.find = vi.fn().mockRejectedValue(new Error('DB error')); const failLoader = new DatabaseLoader({ driver: failingDriver }); - const result = await failLoader.loadMany('object'); - expect(result).toEqual([]); + await expect(failLoader.loadMany('object')).rejects.toThrow('DB error'); }); - it('should return false on exists failure', async () => { + it('should rethrow a read failure from exists', async () => { const failingDriver = createMockDriver(); failingDriver.count = vi.fn().mockRejectedValue(new Error('DB error')); const failLoader = new DatabaseLoader({ driver: failingDriver }); - expect(await failLoader.exists('object', 'account')).toBe(false); + await expect(failLoader.exists('object', 'account')).rejects.toThrow('DB error'); }); - it('should return null on stat failure', async () => { + it('should rethrow a read failure from stat', async () => { const failingDriver = createMockDriver(); failingDriver.findOne = vi.fn().mockRejectedValue(new Error('DB error')); const failLoader = new DatabaseLoader({ driver: failingDriver }); - expect(await failLoader.stat('object', 'account')).toBeNull(); + await expect(failLoader.stat('object', 'account')).rejects.toThrow('DB error'); }); - it('should return empty array on list failure', async () => { + it('should rethrow a read failure from list', async () => { const failingDriver = createMockDriver(); failingDriver.find = vi.fn().mockRejectedValue(new Error('DB error')); const failLoader = new DatabaseLoader({ driver: failingDriver }); - expect(await failLoader.list('object')).toEqual([]); + await expect(failLoader.list('object')).rejects.toThrow('DB error'); }); it('should throw descriptive error on save failure', async () => { @@ -836,6 +843,235 @@ describe('DatabaseLoader event_seq on a failed history read (#4825)', () => { }); }); +// ---------- A storage read failure is never answered as "nothing declared" ---------- + +/** + * #5108 (rule: #4632; same shape one layer up from #4825, ADR-0110 D3). + * + * Every read method used to `catch {}` into its own empty value, so a + * `sys_metadata` the metadata plane could not reach returned EXACTLY what "this + * environment declares nothing of that type" returns — `[]`, `false`, `null` — + * with not one line logged anywhere on the chain. `MetadataManager`'s own + * degradation branches could not fire either, because the loader handed them a + * *successful* empty read rather than an exception. + * + * Both directions are pinned, deliberately, exactly as #4728/#4825 pin theirs: + * proving the outage is loud is not enough, because "always throw" would pass + * that alone while making a first boot against an unprovisioned table explode. + * The point is that the two are DISTINGUISHED. + */ +describe('DatabaseLoader read failures are outages, not misses (#5108)', () => { + /** Benign: nothing provisioned yet, so "nothing declared" really is true. */ + const noSuchTable = () => + Object.assign(new Error('no such table: sys_metadata'), { code: 'SQLITE_ERROR' }); + + /** NOT benign: the rows are there, this read simply did not see them. */ + const connectionReset = () => + Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + + /** + * A driver whose reads of `sys_metadata` fail the way a real outage does. + * `syncSchema` still succeeds — the table was provisioned at boot and the + * datasource fell over afterwards, which is what makes the defect invisible. + */ + function driverWithFailingReads(makeError: () => unknown): IDataDriver { + const driver = createMockDriver(); + driver.find = vi.fn().mockImplementation(() => Promise.reject(makeError())); + driver.findOne = vi.fn().mockImplementation(() => Promise.reject(makeError())); + driver.count = vi.fn().mockImplementation(() => Promise.reject(makeError())); + return driver; + } + + describe('a REAL outage — the driver is reachable but the read failed', () => { + let loader: DatabaseLoader; + + beforeEach(() => { + loader = new DatabaseLoader({ driver: driverWithFailingReads(connectionReset) }); + }); + + it('loadMany rethrows instead of answering []', async () => { + await expect(loader.loadMany('permission')).rejects.toThrow('read ECONNRESET'); + }); + + it('exists rethrows instead of answering false', async () => { + await expect(loader.exists('permission', 'admin_all')).rejects.toThrow('read ECONNRESET'); + }); + + it('stat rethrows instead of answering null', async () => { + await expect(loader.stat('permission', 'admin_all')).rejects.toThrow('read ECONNRESET'); + }); + + it('list rethrows instead of answering []', async () => { + await expect(loader.list('permission')).rejects.toThrow('read ECONNRESET'); + }); + + it('load rethrows too — ADR-0110 D3 needs the singular read to fail loudly', async () => { + await expect(loader.load('permission', 'admin_all')).rejects.toThrow('read ECONNRESET'); + }); + + it('carries the driver error unchanged, so the cause is diagnosable', async () => { + const thrown = await loader.loadMany('permission').catch((e: unknown) => e); + expect((thrown as { code?: string }).code).toBe('ECONNRESET'); + }); + + it('does not poison the cache with the failed read', async () => { + await expect(loader.loadMany('permission')).rejects.toThrow(); + // A retry must hit the driver again rather than serve a memoized []. + await expect(loader.loadMany('permission')).rejects.toThrow(); + }); + }); + + describe('the benign case — `sys_metadata` has not been provisioned yet', () => { + let loader: DatabaseLoader; + + beforeEach(() => { + loader = new DatabaseLoader({ driver: driverWithFailingReads(noSuchTable) }); + }); + + it('answers empty rather than exploding on a first boot', async () => { + await expect(loader.loadMany('permission')).resolves.toEqual([]); + await expect(loader.list('permission')).resolves.toEqual([]); + await expect(loader.exists('permission', 'admin_all')).resolves.toBe(false); + await expect(loader.stat('permission', 'admin_all')).resolves.toBeNull(); + await expect(loader.load('permission', 'admin_all')).resolves.toMatchObject({ data: null }); + }); + + it('does not memoize the empty answer — the table may appear next call', async () => { + const driver = createMockDriver(); + let provisioned = false; + const realFind = driver.find as unknown as (t: string, q: unknown) => Promise[]>; + driver.find = vi.fn().mockImplementation((table: string, query: unknown) => { + if (!provisioned) return Promise.reject(noSuchTable()); + return realFind(table, query); + }); + const healing = new DatabaseLoader({ driver }); + + expect(await healing.list('permission')).toEqual([]); + + provisioned = true; + await healing.save('permission', 'admin_all', { name: 'admin_all' }); + expect(await healing.list('permission')).toEqual(['admin_all']); + }); + }); + + it('DISTINGUISHES the two: same call site, opposite verdicts', async () => { + const benign = new DatabaseLoader({ driver: driverWithFailingReads(noSuchTable) }); + const real = new DatabaseLoader({ driver: driverWithFailingReads(connectionReset) }); + + expect(await benign.loadMany('permission')).toEqual([]); + await expect(real.loadMany('permission')).rejects.toThrow('read ECONNRESET'); + }); + + describe('what the manager on top of it can finally say', () => { + beforeEach(() => { + logger.error.mockClear(); + logger.info.mockClear(); + logger.warn.mockClear(); + }); + + function managerOverBrokenDb(): MetadataManager { + const manager = new MetadataManager({ formats: ['json'], loaders: [new MemoryLoader()] }); + manager.registerLoader( + new DatabaseLoader({ driver: driverWithFailingReads(connectionReset) }), + ); + return manager; + } + + it('list() keeps serving what it can, and reports the outage at `error`', async () => { + const manager = managerOverBrokenDb(); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + + const items = await manager.list('permission'); + // Best-effort listing survives — that posture is deliberate. + expect(items).toEqual([{ name: 'from_code' }]); + + // …but it is no longer silent. AGENTS.md → "Degradation log levels": + // the system looks normal while the set it gates on is short → `error`. + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.warn).not.toHaveBeenCalled(); + const [message] = logger.error.mock.calls[0] as [string]; + expect(message).toContain('database'); + expect(message).toContain('permission'); + expect(message).toMatch(/PARTIAL/); + expect(message).toMatch(/never declared/i); + expect(message).toMatch(/reporting healthy/i); + expect(message).toMatch(/Fix:/); + }); + + it('says it once per outage, not once per read', async () => { + const manager = managerOverBrokenDb(); + + await manager.list('permission'); + await manager.list('view'); + await manager.list('flow'); + + expect(logger.error).toHaveBeenCalledTimes(1); + }); + + it('loadDiagnosed reports `degraded` — ADR-0110 D3 now holds for the DB loader', async () => { + const manager = managerOverBrokenDb(); + + const diagnosed = await manager.loadDiagnosed('permission', 'admin_all'); + expect(diagnosed.data).toBeNull(); + expect(diagnosed.degraded).toBe(true); + expect(diagnosed.errors.join()).toContain('read ECONNRESET'); + }); + + it('a clean miss is still NOT degraded — the distinction is the point', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [new MemoryLoader()] }); + manager.registerLoader(new DatabaseLoader({ driver: createMockDriver() })); + + const diagnosed = await manager.loadDiagnosed('permission', 'never_declared'); + expect(diagnosed.data).toBeNull(); + expect(diagnosed.degraded).toBe(false); + expect(logger.error).not.toHaveBeenCalled(); + }); + + /** + * The reason #5108 was split out of #5089. `listForIndex()` was written + * without a `try/catch` precisely so an unreadable store could not be + * served as "no endpoint declares this route" — but that only worked for + * loaders that actually report their failures. Against a real + * `DatabaseLoader` the seam was inert: the loader swallowed first, so + * `matchEndpoint` answered `undefined`, which the REST layer turns into a + * 404 — an availability failure rendered as a semantic "not declared". + */ + it('matchEndpoint REJECTS on a broken DatabaseLoader instead of 404-shaped undefined', async () => { + const manager = managerOverBrokenDb(); + + await expect( + manager.matchEndpoint({ method: 'GET', path: '/api/v1/apps/showcase/tasks' }), + ).rejects.toThrow('read ECONNRESET'); + }); + + it('…even when the endpoint IS declared in another, healthy loader', async () => { + const manager = managerOverBrokenDb(); + manager.registerInMemory('api', 'list_tasks', { + name: 'list_tasks', + path: '/api/v1/apps/showcase/tasks', + method: 'GET', + type: 'object_operation', + target: 'showcase_task', + }); + + // A partial read cannot prove the match it found is the right one. + await expect( + manager.matchEndpoint({ method: 'GET', path: '/api/v1/apps/showcase/tasks' }), + ).rejects.toThrow('read ECONNRESET'); + }); + + it('an unprovisioned table is NOT an outage — matchEndpoint still answers a clean miss', async () => { + const manager = new MetadataManager({ formats: ['json'], loaders: [new MemoryLoader()] }); + manager.registerLoader(new DatabaseLoader({ driver: driverWithFailingReads(noSuchTable) })); + + await expect( + manager.matchEndpoint({ method: 'GET', path: '/api/v1/apps/showcase/tasks' }), + ).resolves.toBeUndefined(); + expect(logger.error).not.toHaveBeenCalled(); + }); + }); +}); + // ---------- DatabaseLoader read-through cache ---------- describe('DatabaseLoader read-through cache', () => { diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index dbb22b338c..4b32e28320 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -664,6 +664,59 @@ export class DatabaseLoader implements MetadataLoader { }; } + // ========================================== + // Read-failure classification (#5108) + // ========================================== + + /** + * Decide what a failed READ against {@link tableName} means, and rethrow + * unless it is the ONE benign reason. + * + * #5108 (rule from #4632; same shape as #4728 and #4825) — discriminate by + * error TYPE. Every read method below used to `catch {}` into its own empty + * value: `load` → `null`, `loadMany` → `[]`, `exists` → `false`, `stat` → + * `null`, `list` → `[]`. That made a database the metadata plane cannot + * reach **indistinguishable** from an environment where nothing of that type + * was ever declared — and it erased the failure *inside the loader*, so + * neither `MetadataManager`'s own `try/catch` degradation branches nor + * {@link import('../metadata-manager.js').MetadataManager.loadDiagnosed} + * (ADR-0110 D3, whose whole purpose is to tell a miss from an outage) could + * report anything. Nowhere on the chain was there a line saying the read + * failed. + * + * Why that is worse than a noisy error: every consumer that gates on a + * *declared set* — permissions, sharing rules, policies, endpoint + * declarations — reads the empty answer as "the author declared none". Some + * then fail open (grant), some fail closed (lock out); both look healthy + * from outside. This is the AGENTS.md → "Degradation log levels" shape the + * repo has already paid for twice, one layer up from #4825. + * + * Exactly one failure reason is benign: `sys_metadata` has not been + * provisioned yet. There are then genuinely no rows, so "nothing declared" + * IS the truth, and a first boot must not explode. Every other reason — + * connection drop, timeout, insufficient privileges, malformed query — means + * the rows may well be there and simply were not seen. + * + * Classification is conservative in the same direction as + * {@link isMissingTableError} itself: an unrecognised error is NOT benign. + * A false "benign" silently mis-answers a security question; a false "real" + * costs one loud error. + * + * @param error The value thrown by `_find` / `_findOne` / `_count`. + * @throws The underlying driver error, unchanged — deliberately, matching + * {@link nextEventSeq}. The loader does not log it: the caller owns + * the consequence and is the only layer that knows what an + * incomplete answer costs it (`MetadataManager.list()` reports it at + * `error`; `listForIndex()`/`matchEndpoint` let it propagate so an + * outage can never be served as a 404). + * @returns normally ONLY for the benign case, licensing the caller to answer + * with its empty value. + */ + private rethrowUnlessTableUnprovisioned(error: unknown): void { + if (isMissingTableError(error)) return; + throw error; + } + // ========================================== // MetadataLoader Interface Implementation // ========================================== @@ -718,7 +771,11 @@ export class DatabaseLoader implements MetadataLoader { etag: record.checksum, loadTime: Date.now() - startTime, }; - } catch { + } catch (error) { + this.rethrowUnlessTableUnprovisioned(error); + // Benign only: the table is not provisioned, so there is no row. Not + // cached — `ensureSchema()` retries, and a `null` memoized here would + // outlive the provisioning that fixes it. return { data: null, loadTime: Date.now() - startTime, @@ -748,7 +805,9 @@ export class DatabaseLoader implements MetadataLoader { this.loadManyCache?.set(type, result); return result; - } catch { + } catch (error) { + this.rethrowUnlessTableUnprovisioned(error); + // Benign only: no table, therefore no items of this type. Not cached. return []; } } @@ -768,7 +827,9 @@ export class DatabaseLoader implements MetadataLoader { }); return count > 0; - } catch { + } catch (error) { + this.rethrowUnlessTableUnprovisioned(error); + // Benign only: no table, therefore the item genuinely does not exist. return false; } } @@ -805,7 +866,9 @@ export class DatabaseLoader implements MetadataLoader { }; this.statCache?.set(key, stats); return stats; - } catch { + } catch (error) { + this.rethrowUnlessTableUnprovisioned(error); + // Benign only: no table, therefore nothing to stat. Not cached. return null; } } @@ -830,7 +893,9 @@ export class DatabaseLoader implements MetadataLoader { this.listCache?.set(type, names); return names; - } catch { + } catch (error) { + this.rethrowUnlessTableUnprovisioned(error); + // Benign only: no table, therefore no names. Not cached. return []; } } diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index c794e8a601..c9b41589a8 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -149,6 +149,14 @@ export class MetadataManager implements IMetadataService { private listCache = new Map(); private static readonly LIST_CACHE_TTL_MS = 30_000; + // [#5108] Loader names whose read failure has already been reported at + // `error` by `list()`. AGENTS.md → "Degradation log levels": say it once, at + // the first degradation — `list()` is hot enough that one line per failed + // read would bury the one line that matters. Cleared when the loader answers + // again, so a second outage is reported again. Same once-only discipline as + // `DatabaseLoader.schemaFailureReported`. + private readonly loaderReadFailureReported = new Set(); + // Realtime service for event publishing private realtimeService?: IRealtimeService; @@ -536,8 +544,9 @@ export class MetadataManager implements IMetadataService { items.set(itemAny.name, item); } } + this.reportLoaderReadRecovered(loader.contract.name); } catch (e) { - this.logger.warn(`Loader ${loader.contract.name} failed to loadMany ${type}`, { error: e }); + this.reportLoaderReadFailure(loader.contract.name, type, e); } } @@ -545,6 +554,60 @@ export class MetadataManager implements IMetadataService { this.cacheListResult(type, result); return result; } + + /** + * Report — at `error`, once per outage episode — that a loader could not be + * read while serving {@link list}. + * + * [#5108] This branch used to be dead for the loader that matters. Before + * #5108 `DatabaseLoader` caught its own read failures and answered `[]`, so + * `list()` received a *successful empty read* and never entered this `catch` + * at all: an unreachable `sys_metadata` and "this environment declares no + * `permission`" produced byte-identical results with not one line logged. + * With the loader rethrowing everything but the benign not-provisioned case, + * this is where the outage finally becomes speakable. + * + * `error`, not `warn`, per AGENTS.md → "Degradation log levels". Apply its + * one question — *does the system still look normal from outside while + * something it claims to know has not actually landed?* — and the answer is + * yes: `list()` still returns, callers still get an array, nothing 500s, and + * the set they gate on is quietly short. Which way that cuts depends on the + * consumer, and both ways are silent (#3935 is the fail-open precedent). + * + * Said **once** per loader, and un-said on recovery, because `list()` is a + * hot path — one line per outage, not one per read. + * + * 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 + * `catch`, feeding `matchEndpoint`) and {@link loadDiagnosed} (ADR-0110 D3) + * for the singular read — both of which only became honest for + * `DatabaseLoader` with the same #5108 change. + */ + private reportLoaderReadFailure(loaderName: string, type: string, error: unknown): void { + if (this.loaderReadFailureReported.has(loaderName)) return; + this.loaderReadFailureReported.add(loaderName); + this.logger.error( + `[MetadataManager] Loader \`${loaderName}\` could NOT be read (first failure seen while listing \`${type}\`) — ` + + `every list served from now on is a PARTIAL set presented as a complete one, and the server keeps reporting healthy. ` + + `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.`, + error instanceof Error ? error : undefined, + { loader: loaderName, type, error }, + ); + } + + /** Un-say {@link reportLoaderReadFailure} once the loader answers again. */ + private reportLoaderReadRecovered(loaderName: string): void { + if (!this.loaderReadFailureReported.delete(loaderName)) return; + this.logger.info( + `[MetadataManager] Loader \`${loaderName}\` is readable again — listings are complete once more.`, + ); + } + private cacheListResult(type: string, items: unknown[]): void { this.listCache.set(type, { ts: Date.now(), items }); } @@ -565,7 +628,7 @@ export class MetadataManager implements IMetadataService { * Enumerate stored items of `type` for an index build — like {@link list}, * but a store that cannot be read THROWS instead of contributing nothing. * - * [#5089] `list()` deliberately warn-logs and skips a failing loader so a + * [#5089] `list()` deliberately logs a failing loader and skips it so a * partially-available metadata plane still serves what it can. That posture * is wrong for `matchEndpoint`: its `undefined` becomes an HTTP 404, and a * store outage that silently yields "zero declarations" would turn every @@ -577,10 +640,14 @@ export class MetadataManager implements IMetadataService { * is `list()`'s failure posture inverted for the one caller whose answer is * a security/availability decision rather than a best-effort listing. * - * ⚠️ This surfaces only failures a loader actually reports. `DatabaseLoader` - * currently swallows its own read errors into `[]` (#5108), so a DB outage is - * invisible even here — that is a defect in the loader, not a reason to - * soften this seam. + * This surfaces only failures a loader actually reports — which, since + * #5108, includes `DatabaseLoader`: it used to swallow its own read errors + * into `[]`, making a DB outage invisible even here. It now rethrows every + * read failure except the benign "table not provisioned yet", so this seam + * holds against the real datasource-backed loader and not just the memory / + * remote ones. (`database-loader.test.ts` pins that end to end: a broken + * driver behind a real `DatabaseLoader` makes `matchEndpoint` reject rather + * than answer a 404-shaped `undefined`.) */ private async listForIndex(type: string): Promise { const items = new Map(); From d81d24d244225f4a3f6298cbb73c06ff2f0c681d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 08:04:31 +0000 Subject: [PATCH 2/2] fix(metadata): route MetadataManager.loadMany's outage report through the same seam (#5108) `list()` and `loadMany()` catch the identical loader failure two methods apart. Leaving one at `warn` while `list()` moves to `error` puts two levels on one storage outage in one file, which is how the wrong one gets copied. Both now call `reportLoaderReadFailure`, so the level, the consequence-and-fix text and the say-it-once/un-say-on-recovery discipline are declared in a single place. No behavioural change beyond the log level: `loadMany()` still returns what the reachable loaders held. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 --- ...tabase-loader-read-outage-is-not-a-miss.md | 4 ++- .../src/loaders/database-loader.test.ts | 28 +++++++++++++++++++ packages/metadata/src/metadata-manager.ts | 7 ++++- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/.changeset/database-loader-read-outage-is-not-a-miss.md b/.changeset/database-loader-read-outage-is-not-a-miss.md index e50f49f693..b81c3b875d 100644 --- a/.changeset/database-loader-read-outage-is-not-a-miss.md +++ b/.changeset/database-loader-read-outage-is-not-a-miss.md @@ -25,7 +25,9 @@ fix(metadata): `DatabaseLoader` 的读故障不再被吞成「什么都没声明 - `MetadataManager.list()` 的降级分支会真的进,并且**升级到 `error`** (AGENTS.md「Degradation log levels」:系统看着正常、它声称掌握的清单其实是残缺的), 日志写明后果与修法,每次故障只说一次、恢复时再说一次;`list()` 仍然尽力返回可读 - loader 的内容 —— 这个 best-effort 姿态是刻意保留的; + loader 的内容 —— 这个 best-effort 姿态是刻意保留的。兄弟方法 + `MetadataManager.loadMany()` 的同一条缝走同一个判决,不让同一次故障在同一个文件里 + 报出两个级别; - `MetadataManager.loadDiagnosed()`(ADR-0110 D3)对 `DatabaseLoader` 终于能报出 `degraded` / `errors`,而不是把 outage 报成 miss; - `listForIndex()` / `matchEndpoint`(#5089)契约要求「读不到存储必须抛出,不得伪装成 diff --git a/packages/metadata/src/loaders/database-loader.test.ts b/packages/metadata/src/loaders/database-loader.test.ts index daea589793..1579c1d23c 100644 --- a/packages/metadata/src/loaders/database-loader.test.ts +++ b/packages/metadata/src/loaders/database-loader.test.ts @@ -1008,6 +1008,34 @@ describe('DatabaseLoader read failures are outages, not misses (#5108)', () => { expect(logger.error).toHaveBeenCalledTimes(1); }); + it('the sibling plural read, loadMany(), reports at the same level', async () => { + const manager = managerOverBrokenDb(); + + await expect(manager.loadMany('permission')).resolves.toEqual([]); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('un-says it when the loader becomes readable again', async () => { + const driver = createMockDriver(); + let broken = true; + const realFind = driver.find as unknown as (t: string, q: unknown) => Promise[]>; + driver.find = vi.fn().mockImplementation((table: string, query: unknown) => { + if (broken) return Promise.reject(connectionReset()); + return realFind(table, query); + }); + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerLoader(new DatabaseLoader({ driver, cache: { enabled: false } })); + + await manager.loadMany('permission'); + expect(logger.error).toHaveBeenCalledTimes(1); + + broken = false; + await manager.loadMany('permission'); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(logger.info.mock.calls.map(c => c[0]).join()).toMatch(/readable again/i); + }); + it('loadDiagnosed reports `degraded` — ADR-0110 D3 now holds for the DB loader', async () => { const manager = managerOverBrokenDb(); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index c9b41589a8..434448ff2d 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -1701,8 +1701,13 @@ export class MetadataManager implements IMetadataService { } results.push(item); } + this.reportLoaderReadRecovered(loader.contract.name); } catch (e) { - this.logger.warn(`Loader ${loader.contract.name} failed to loadMany ${type}`, { error: e }); + // [#5108] Same seam, same verdict as `list()` — see + // {@link reportLoaderReadFailure}. Two adjacent plural reads + // reporting one storage outage at two different levels is how the + // wrong one gets copied. + this.reportLoaderReadFailure(loader.contract.name, type, e); } } return results;