From 9fe81081fdfaa1b834153b253aa4d3b06bde98c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 12:11:37 +0000 Subject: [PATCH] feat(spec,metadata,mcp): let a plural metadata read say it is known-partial (#6504) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IMetadataService.list(type)` returns an array whether every loader answered or one was down, so a consumer receiving a short list could not ask which it was. `MetadataManager.readListUncached()` has computed the `degraded` verdict since #5184 and `list()` spent it entirely on a cache TTL. This is the #5840 / PR #6051 shape on the plural read, and sharper there: `list` is the read whose answer carries a count, and a count is the strongest positive claim a read can make. - spec: new optional `IMetadataService.listDiagnosed?(type)` returning `{ items, degraded, errors }` — the plural counterpart of `getDiagnosed`. - metadata: `MetadataManager.listDiagnosed()`, sharing `list()`'s cache entry and single-flight slot, so the verdict costs no extra loader walk and the two members cannot drift. `list()` is unchanged in every direction. - mcp: the two measured consumers, classified individually per PR #6051. `objectstack://objects` mis-described, so a degraded read now withholds `totalCount` while still serving the objects it could read; the skill bridge is a snapshot and reports its incompleteness to the operator instead. Part of #6504 — the wider consumer sweep (metadata-protocol, rest, runtime, plugins) is deliberately excluded while #7674 is in flight on `packages/metadata-protocol/src/protocol.ts`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RoUxMErFzTQVpQzjNgDAGm --- .changeset/metadata-list-diagnosed.md | 56 +++ .../mcp-server-runtime.list-outage.test.ts | 310 ++++++++++++++++ packages/mcp/src/mcp-server-runtime.ts | 249 ++++++++++++- .../metadata-manager-list-diagnosed.test.ts | 337 ++++++++++++++++++ packages/metadata/src/metadata-manager.ts | 116 +++++- .../spec/src/contracts/metadata-service.ts | 53 +++ 6 files changed, 1095 insertions(+), 26 deletions(-) create mode 100644 .changeset/metadata-list-diagnosed.md create mode 100644 packages/mcp/src/mcp-server-runtime.list-outage.test.ts create mode 100644 packages/metadata/src/metadata-manager-list-diagnosed.test.ts diff --git a/.changeset/metadata-list-diagnosed.md b/.changeset/metadata-list-diagnosed.md new file mode 100644 index 0000000000..2a9f1356c8 --- /dev/null +++ b/.changeset/metadata-list-diagnosed.md @@ -0,0 +1,56 @@ +--- +"@objectstack/spec": minor +"@objectstack/metadata": minor +"@objectstack/mcp": minor +--- + +feat(spec,metadata,mcp): let a plural metadata read say it is known-partial (#6504) + +`IMetadataService.list(type)` returns an array whether every loader answered or +one of them was down. A consumer receiving a short list therefore had no way to +ask whether it was short because that is all anyone declared, or because a +loader was unreachable — the #5840 / PR #6051 defect on the plural read. + +The verdict already existed and was already being thrown away. +`MetadataManager.readListUncached()` has computed a `degraded` flag since #5184, +and `list()` spent it entirely on picking a cache TTL. This is sharper than the +singular case rather than merely analogous: `list` is the read whose answer +carries a **count**, and a consumer restating `items.length` as "this +environment contains N items" makes a positive, numeric claim out of a read that +partly did not happen. + +**New optional contract member — `listDiagnosed?(type)`.** Returns +`{ items, degraded, errors }`, the plural counterpart of `getDiagnosed`. +Optional for the same reason its singular twin is: an implementation that +predates it cannot report the distinction, so a consumer probes for it and falls +back to `list()`, which reports nothing degraded. `list()` itself is unchanged +in every direction — same items, same array instance, same best-effort posture — +so no existing caller has to do anything. + +`MetadataManager` implements it through the same cache entry and the same +single-flight slot `list()` uses, so asking for the verdict costs no extra +loader walk and the two members cannot drift. + +**MCP consumers, classified individually** (PR #6051's discipline, not a blanket +switch): + +- `objectstack://objects` **mis-described**, and its degraded body changes. It + rendered `{ objects, totalCount }`, and during an outage `totalCount` was + simply false. A healthy read is byte-identical to before. A degraded read now + serves the same `objects` — the reachable set is still the most useful true + thing here — with `totalCount` **absent** and `partial: true`, + `returnedCount`, `warning`, plus the `code: 'SERVICE_UNAVAILABLE'` / `status: + 503` envelope the sibling `objectstack://objects/{objectName}` resource + already carries. Dropping the key rather than reporting a smaller number is + the point: a client reading `body.totalCount` now gets `undefined`, where a + plausible-looking integer would have been believed. +- the `agent_prompt` sibling **skill bridge** is a snapshot and its output is + unchanged. It publishes no count to any client, so a degraded read costs it + silently-unregistered prompts instead of a false statement; the verdict goes + to the operator as a `warn` naming the loader, the fact that the skills are + missing rather than undeclared, and that the stdio transport's snapshot stays + short until restart while the HTTP transport self-heals. + +If you consume `objectstack://objects` and read `totalCount` unconditionally, +branch on `partial` (or on the key's absence) before treating any count from +this resource as a total. diff --git a/packages/mcp/src/mcp-server-runtime.list-outage.test.ts b/packages/mcp/src/mcp-server-runtime.list-outage.test.ts new file mode 100644 index 0000000000..323f6524e8 --- /dev/null +++ b/packages/mcp/src/mcp-server-runtime.list-outage.test.ts @@ -0,0 +1,310 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6504, ADR-0110 D3 — MCP side, plural read] A metadata plane that could not + * be fully READ is not an environment that declares fewer things. + * + * --------------------------------------------------------------------------- + * The defect + * --------------------------------------------------------------------------- + * `MetadataManager` has computed a `degraded` verdict for every `list()` since + * #5184 and spent it entirely on a cache TTL. The two consumers in this file + * therefore read a short array during a loader outage with no way to ask why it + * was short — the plural instance of the #5840 / PR #6051 shape #6055 closed on + * the singular read. + * + * The two are NOT the same case, and PR #6051's discipline is to classify each + * consumer rather than apply one rule, so they are pinned separately: + * + * - `objectstack://objects` **MIS-DESCRIBES**. It renders the listing as + * `{ objects, totalCount }`, and during an outage `totalCount` is a + * positive, numeric claim that is simply false. This is the surface where a + * count is the strongest thing a read can wrongly say, and the fix withholds + * the count while still serving the objects. + * - the `agent_prompt` sibling **skill bridge** produces a SNAPSHOT. It + * publishes no count and makes no completeness claim to any client; a + * degraded read costs it silently-unregistered prompts. There is nobody on + * the wire to tell, so the verdict goes to the operator at `warn`. + * + * --------------------------------------------------------------------------- + * Why these doubles, and where the REAL loader failure is pinned + * --------------------------------------------------------------------------- + * Stated plainly rather than papered over. `packages/mcp` depends on + * `@objectstack/spec`, `core`, `types` and `formula` — deliberately NOT on + * `@objectstack/metadata` — so a real `MetadataManager` over a broken + * `DatabaseLoader` cannot be constructed here, and adding that dependency to + * drive a test would be a larger architectural change than the fix. + * + * The real failure is therefore driven where the loader actually lives: + * `packages/metadata/src/metadata-manager-list-diagnosed.test.ts` fails a + * `DatabaseLoader`'s driver for real, so `readListUncached()`'s `catch` is what + * produces `degraded`, and asserts the exact record shape the doubles below + * return. This file pins the other half of that chain — what each consumer DOES + * with such a record — which is the same split #6055 used for `getDiagnosed`. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Ordinary red, taken on this consumer. These doubles feed `listDiagnosed`'s + * return contract directly, so reverting the producer cannot move this file — + * only restoring the pre-#6504 reads here can. + * + * The reversion is defined **behaviourally, not textually**: the degraded + * branch of `buildObjectListResource` is removed so it answers + * `{ objects, totalCount }` unconditionally, and the skill bridge's + * incompleteness `warn` is removed. The extraction of the builder is kept. + * Reverting the whole FILE to `origin/main` instead would delete the builder's + * export, fail the import, and turn all eleven cases red — a result that + * measures the extraction rather than the decision, and so proves nothing about + * either. + * + * Predicted, written down before running: **5 red / 6 green**. + * + * The six predicted GREEN are invariant pins rather than gaps, and each is + * green in BOTH directions on purpose: + * - the two HEALTHY cases (resource + skill bridge) — this fix deliberately + * leaves the healthy answer byte-identical, so a red there would report a + * regression, not the fix; + * - *"a degraded listing still serves the objects it could read"* — the + * pre-fix code served them too. It is what would go red if a future change + * here started withholding data instead of withholding the claim; + * - *"the readable skills are still bridged"* — same shape, on the snapshot; + * - the two *"a service without listDiagnosed"* cases — the optional-member + * fallback, which by construction resolves to the pre-fix behaviour. + * The measured result is recorded in the PR body as it came out. + * + * The doubles declare metadata reads only — no engine write verb — so there is + * no `delete`/`update` dispatch for `check:engine-double-contract` to scan and + * no guard to hand-mirror. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IMetadataService, Logger } from '@objectstack/spec/contracts'; +import { MCPServerRuntime, buildObjectListResource } from './mcp-server-runtime.js'; + +type AnyRecord = Record; + +/** + * A logger double that is BOTH a real `Logger` and a set of vitest mocks. + * + * Typed rather than left as a bare record on purpose: this package's tsconfig + * excludes `*.test.ts`, so `pnpm typecheck` never reads this file and only the + * TEST_DEBT ratchet does. An untyped double compiles to eight TS2345s that the + * ledger's surplus would have absorbed in silence — which is the exact shape + * #6376 exists to stop. + */ +type MockLogger = Logger & { + debug: ReturnType; + info: ReturnType; + warn: ReturnType; + error: ReturnType; +}; + +const LOADER_FAILURE = 'database: connect ECONNREFUSED 10.0.0.5:5432'; + +/** What the loader that IS reachable holds — one object, one skill. */ +const REACHABLE_OBJECT = { name: 'acct', label: 'Account', fields: { title: { type: 'text' } } }; +const REACHABLE_SKILL = { + name: 'case_management', + label: 'Case Management', + instructions: 'Handle the support case lifecycle.', +}; + +function makeLogger(): MockLogger { + return { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() } as unknown as MockLogger; +} + +/** Everything logged at `warn`, joined — the snapshot cases read this. */ +const warnLines = (logger: MockLogger): string => + logger.warn.mock.calls.map((c: unknown[]) => String(c[0])).join('\n'); + +/** + * Build a metadata-service double. + * + * Every REQUIRED member of `IMetadataService` is present and throws, so a code + * path that reaches one this fix should not touch fails loudly instead of + * resolving an empty array and looking like the very shortness under test. + */ +function makeService(overrides: AnyRecord): IMetadataService { + const unexpected = (member: string) => async (): Promise => { + throw new Error(`double: ${member}() should not be called by this surface`); + }; + return { + register: unexpected('register'), + get: unexpected('get'), + list: unexpected('list'), + unregister: unexpected('unregister'), + exists: unexpected('exists'), + listNames: unexpected('listNames'), + getObject: unexpected('getObject'), + listObjects: unexpected('listObjects'), + ...overrides, + } as unknown as IMetadataService; +} + +/** + * One loader is down: the reachable ones answered, so `items` is a real + * best-effort set and `degraded` says it is short. This is the record + * `MetadataManager.listDiagnosed()` returns from a live `ECONNREFUSED`, pinned + * in the metadata package. + */ +const listInOutage = (items: unknown[]) => + makeService({ + listObjects: vi.fn(async () => items), + list: vi.fn(async () => items), + listDiagnosed: vi.fn(async () => ({ items, degraded: true, errors: [LOADER_FAILURE] })), + }); + +/** Every loader answered — the same items, and nothing degraded. */ +const listComplete = (items: unknown[]) => + makeService({ + listObjects: vi.fn(async () => items), + list: vi.fn(async () => items), + listDiagnosed: vi.fn(async () => ({ items, degraded: false, errors: [] })), + }); + +/** A service predating #6504: no `listDiagnosed` to probe. */ +const listUndiagnosable = (items: unknown[]) => + makeService({ + listObjects: vi.fn(async () => items), + list: vi.fn(async () => items), + }); + +/** The parsed JSON body of the one-content resource answer. */ +const bodyOf = async (result: { contents: Array<{ text: string }> }): Promise => + JSON.parse(result.contents[0]!.text) as AnyRecord; + +describe('#6504 — `objectstack://objects` MIS-DESCRIBES: the count is withheld, the objects are not', () => { + it('HEALTHY: `totalCount` is served exactly as before', async () => { + const body = await bodyOf( + await buildObjectListResource(listComplete([REACHABLE_OBJECT]), makeLogger()), + ); + + expect(body.totalCount).toBe(1); + expect(body.objects).toHaveLength(1); + expect(body.objects[0].name).toBe('acct'); + // No degradation vocabulary on a complete read. + expect(body.partial).toBeUndefined(); + expect(body.code).toBeUndefined(); + }); + + it('DEGRADED: the count claim is ABSENT — a client asking for a total gets nothing, not a wrong number', async () => { + const body = await bodyOf( + await buildObjectListResource(listInOutage([REACHABLE_OBJECT]), makeLogger()), + ); + + // The whole point. Before #6504 this said `totalCount: 1` about an + // environment that declares more, and a machine consumer believed it. + expect(body.totalCount).toBeUndefined(); + expect('totalCount' in body).toBe(false); + + // What replaces it says only what is known, in a name that cannot be read + // as a total. + expect(body.partial).toBe(true); + expect(body.returnedCount).toBe(1); + }); + + it('DEGRADED: carries the ADR-0112-shaped envelope the sibling resource already uses', async () => { + const body = await bodyOf( + await buildObjectListResource(listInOutage([REACHABLE_OBJECT]), makeLogger()), + ); + + expect(body.code).toBe('SERVICE_UNAVAILABLE'); + expect(body.status).toBe(503); + // The prose states the DIRECTION of the error — a floor, never an exact + // number — and does not describe the listing as complete. + expect(body.warning).toMatch(/INCOMPLETE/); + expect(body.warning).toMatch(/at least/i); + expect(body.warning).not.toMatch(/not found/i); + }); + + it('the two answers are not equal — which is the fact the defect WAS', async () => { + const outage = await bodyOf( + await buildObjectListResource(listInOutage([REACHABLE_OBJECT]), makeLogger()), + ); + const complete = await bodyOf( + await buildObjectListResource(listComplete([REACHABLE_OBJECT]), makeLogger()), + ); + + // Same objects, same length — so nothing but the completeness claim moved, + // and yet the two bodies are now distinguishable. + expect(outage.objects).toEqual(complete.objects); + expect(outage).not.toEqual(complete); + }); + + it('DEGRADED: still serves the objects it could read — this is a diagnosis fix, not a withholding one', async () => { + const body = await bodyOf( + await buildObjectListResource(listInOutage([REACHABLE_OBJECT]), makeLogger()), + ); + + // Kept deliberately free of any degradation assertion: this case pins the + // AFFORDANCE, which the pre-#6504 code had too. It cannot go red on this + // fix's reversion, and it is what would go red if a future change here + // started withholding data instead of withholding the claim. + expect(body.objects).toHaveLength(1); + expect(body.objects[0].name).toBe('acct'); + }); + + it('DEGRADED: the operator is told once, naming the loader that was lost', async () => { + const logger = makeLogger(); + await buildObjectListResource(listInOutage([REACHABLE_OBJECT]), logger); + + expect(warnLines(logger)).toMatch(/known-partial/); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn.mock.calls[0][1].errors).toEqual([LOADER_FAILURE]); + }); + + it('a service without `listDiagnosed` behaves exactly as before — the member is optional', async () => { + const logger = makeLogger(); + const body = await bodyOf( + await buildObjectListResource(listUndiagnosable([REACHABLE_OBJECT]), logger), + ); + + // An implementation that cannot report the distinction reports nothing + // degraded, which is precisely what it could express. + expect(body.totalCount).toBe(1); + expect(body.partial).toBeUndefined(); + expect(logger.warn).not.toHaveBeenCalled(); + }); +}); + +describe('#6504 — the skill bridge is a SNAPSHOT: the verdict goes to the operator', () => { + const bridgeSkills = async (service: IMetadataService, logger: MockLogger): Promise => { + const runtime = new MCPServerRuntime({ name: 'list-outage', version: '0.0.0', logger }); + await runtime.bridgePrompts(service); + }; + + it('DEGRADED: says the prompt list is incomplete, and that the skills are MISSING rather than undeclared', async () => { + const logger = makeLogger(); + await bridgeSkills(listInOutage([REACHABLE_SKILL]), logger); + + const lines = warnLines(logger); + expect(lines).toMatch(/INCOMPLETE/); + expect(lines).toMatch(/missing, NOT undeclared/); + // The consequence that makes it worth saying: the stdio snapshot outlives + // the outage, so a healed loader does not fix the surface by itself. + expect(lines).toMatch(/restarted/); + }); + + it('DEGRADED: the readable skills are still bridged — the short surface is served, not refused', async () => { + const logger = makeLogger(); + await bridgeSkills(listInOutage([REACHABLE_SKILL]), logger); + + expect(logger.info.mock.calls.map((c: unknown[]) => String(c[0])).join('\n')) + .toMatch(/Bridged 1 skill prompts/); + }); + + it('HEALTHY: no incompleteness is announced', async () => { + const logger = makeLogger(); + await bridgeSkills(listComplete([REACHABLE_SKILL]), logger); + + expect(warnLines(logger)).not.toMatch(/INCOMPLETE/); + }); + + it('a service without `listDiagnosed` announces nothing either — same optionality', async () => { + const logger = makeLogger(); + await bridgeSkills(listUndiagnosable([REACHABLE_SKILL]), logger); + + expect(warnLines(logger)).not.toMatch(/INCOMPLETE/); + }); +}); diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index 87aa516fc1..cf4ae70617 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -113,6 +113,28 @@ function metadataUnavailableSentence(subject: string, withheld: string): string ); } +/** + * [#6504] The sentence for "a listing that is known to be SHORT" — the plural + * counterpart of {@link metadataUnavailableSentence}, and deliberately not the + * same sentence. + * + * The singular one says nothing is being served, because on that surface + * nothing is. Here the best-effort set IS served: a partial listing is still + * the most useful true thing this surface has, and withholding it would turn a + * diagnosis fix into a functional regression. What is withheld is the + * **completeness claim** on top of it — which is the entire defect — so the + * sentence states the direction of the error (`at least`, never exactly) and + * names the count as *served*, never as a total. + */ +function metadataPartialListingSentence(plural: string, served: number): string { + return ( + `The metadata service could not be fully read, so this listing of ${plural} is known to be INCOMPLETE. ` + + `${served} ${served === 1 ? 'is' : 'are'} being served and the total is withheld — ` + + `this environment declares at least that many ${plural}, possibly more. ` + + 'Retry once the metadata service is reachable.' + ); +} + /** What {@link diagnosedGet} and {@link diagnoseEmptyRead} report. */ interface DiagnosedRead { data: unknown; @@ -120,6 +142,84 @@ interface DiagnosedRead { errors: string[]; } +/** What {@link diagnosedList} reports — {@link DiagnosedRead} for a plural read. */ +interface DiagnosedListRead { + items: unknown[]; + degraded: boolean; + errors: string[]; +} + +/** + * [#6504] List one metadata type, keeping the ADR-0110 D3 verdict instead of + * flattening an outage into an array indistinguishable from a small + * environment. + * + * `IMetadataService.listDiagnosed` is **optional** for the reason + * `getDiagnosed` is (#5840): a service that predates it cannot report the + * distinction, so it is read exactly as before and reports nothing degraded. + * Same probe-and-fall-back shape as {@link diagnosedGet}, one read over. + * + * Unlike its singular twin this one is defensive about `items` as well as the + * verdict — `list` may resolve nullish on an implementation that predates the + * non-null guarantee, which is why the call site being replaced carried its own + * `?? []`. + */ +async function diagnosedList( + metadataService: IMetadataService, + type: string, +): Promise { + if (typeof metadataService.listDiagnosed === 'function') { + const diagnosed = await metadataService.listDiagnosed(type); + return { + items: Array.isArray(diagnosed?.items) ? diagnosed.items : [], + degraded: diagnosed?.degraded === true, + errors: Array.isArray(diagnosed?.errors) ? diagnosed.errors : [], + }; + } + return { items: (await metadataService.list(type)) ?? [], degraded: false, errors: [] }; +} + +/** + * [#6504] The verdict for a listing that did NOT go through `list` — used by + * the `objectstack://objects` resource, whose resolver is `listObjects()`. + * + * The plural instance of {@link diagnoseEmptyRead}'s decision, taken on the + * same ground and for the same reason: `listObjects` is its own member of + * `IMetadataService` and declares **no equivalence** to `list('object')`, so + * presuming one at a consumer would be the private dialect Prime Directive #12 + * forbids. The resolver is therefore left untouched and only the *question* + * "could this answer be trusted as complete?" is asked of the member declared + * to answer it. + * + * Two differences from the singular probe, both deliberate: + * + * - It runs on **every** answer, not only an empty one. The defect this closes + * is a non-empty-but-short list rendered with a confident count, so "the + * answer looks fine" is exactly the case that needs asking. + * - That costs a second read per call in principle and nothing in practice on + * the implementation that ships: `MetadataManager.listObjects()` is + * `list('object')`, and `list`/`listDiagnosed` share one cache entry and one + * single-flight slot, so the probe lands on the entry the resolver just + * filled. On a host where the two resolve different sets the verdict + * describes the loader set rather than that host's own listing — which + * withholds a completeness claim it might have been entitled to, and never + * manufactures one it is not. That is the same conservative direction + * {@link diagnoseEmptyRead} accepts. + */ +async function diagnoseListRead( + metadataService: IMetadataService, + type: string, +): Promise<{ degraded: boolean; errors: string[] }> { + if (typeof metadataService.listDiagnosed !== 'function') { + return { degraded: false, errors: [] }; + } + const diagnosed = await metadataService.listDiagnosed(type); + return { + degraded: diagnosed?.degraded === true, + errors: Array.isArray(diagnosed?.errors) ? diagnosed.errors : [], + }; +} + /** * [#6055] Read one metadata item, keeping the ADR-0110 D3 verdict instead of * flattening an outage into the same `undefined` a never-declared name @@ -392,6 +492,88 @@ export async function buildObjectSchemaResource( }; } +/** + * [#6504] Resolve the `objectstack://objects` answer for one call. + * + * Extracted from the resource handler for the reason + * {@link buildAgentPromptResult} was: the handler is registered on a private + * `McpServer`, so driving it over a transport would test the SDK rather than + * this decision. + * + * **This surface MIS-DESCRIBES, and that is why it is treated differently from + * the skill bridge in {@link MCPServerRuntime.bridgePrompts}.** It renders the + * listing as `{ objects, totalCount }`, and `totalCount` is a positive, numeric + * claim about what this environment declares. During a loader outage the claim + * is simply false — an MCP client is told, with a number, that the environment + * contains fewer objects than it does — and nothing in the payload lets it tell + * that from a genuinely small environment. + * + * The fix withholds the CLAIM, not the data: + * + * - **healthy** — `{ objects, totalCount }`, byte-identical to before. A count + * from a complete read is a fact this surface was always right to state. + * - **degraded** — the same `objects` (the best-effort set is still the most + * useful true thing here), and `totalCount` is **absent**. In its place: + * `partial: true`, `returnedCount`, and the `code`/`status` envelope the + * sibling resource already carries, so a client can branch structurally. + * + * Dropping the key rather than reporting a smaller number is the point. A + * client reading `body.totalCount` gets `undefined` — which fails, or renders + * as nothing, or throws — where a plausible-looking integer would have been + * believed. The absent key is the loud version of "we do not know the total"; + * `returnedCount` says the one thing that IS known, in a name that cannot be + * mistaken for a total. + * + * Unchanged in the other direction: this is a diagnosis fix, so a degraded read + * still serves every object it could reach. Withholding them would be a new + * functional regression rather than the removal of a false statement. + */ +export async function buildObjectListResource( + metadataService: IMetadataService, + logger?: Logger, +): Promise { + const objects = await metadataService.listObjects(); + const summary = ((objects ?? []) as ObjectDef[]).map(o => ({ + name: o.name, + label: o.label ?? o.name, + fieldCount: o.fields ? Object.keys(o.fields).length : 0, + })); + + // Asked on every answer, not just an empty one — a short list rendered with a + // confident count is exactly the case that looks fine. See {@link diagnoseListRead}. + const { degraded, errors } = await diagnoseListRead(metadataService, 'object'); + + if (degraded) { + logger?.warn( + '[MCP] object listing served WITHOUT a total — the metadata service could not be fully read, so ' + + 'this listing is known-partial and any count taken from it would understate the environment. ' + + 'The caller was told SERVICE_UNAVAILABLE and given the objects that could be read ' + + '(unchanged: the reachable set is still served). ' + + 'Fix: check the loaders behind the metadata service (datasource connection, credentials, table).', + { returnedCount: summary.length, errors }, + ); + } + + const body = degraded + ? { + objects: summary, + partial: true, + returnedCount: summary.length, + warning: metadataPartialListingSentence('objects', summary.length), + code: METADATA_UNAVAILABLE_CODE, + status: 503, + } + : { objects: summary, totalCount: summary.length }; + + return { + contents: [{ + uri: 'objectstack://objects', + mimeType: 'application/json', + text: JSON.stringify(body, null, 2), + }], + }; +} + /** * MCPServerRuntime — Bridges ObjectStack kernel services to the Model Context Protocol. * @@ -588,22 +770,10 @@ export class MCPServerRuntime { description: 'List all data objects (tables) in the ObjectStack instance', mimeType: 'application/json', }, - async () => { - const objects = await metadataService.listObjects(); - const summary = (objects as ObjectDef[]).map(o => ({ - name: o.name, - label: o.label ?? o.name, - fieldCount: o.fields ? Object.keys(o.fields).length : 0, - })); - - return { - contents: [{ - uri: 'objectstack://objects', - mimeType: 'application/json', - text: JSON.stringify({ objects: summary, totalCount: summary.length }, null, 2), - }], - }; - }, + async () => + // [#6504] The completeness of the count lives in the builder — see + // {@link buildObjectListResource}. + buildObjectListResource(metadataService, logger), ); resourceCount++; @@ -749,8 +919,36 @@ export class MCPServerRuntime { logger?.info('[MCP] Agent prompts bridged'); // ── Skill metadata → MCP prompts (#3905) ── + // [#6504] This consumer is a SNAPSHOT, not a mis-describing surface, and is + // treated accordingly — the per-consumer discipline PR #6051 established, + // applied rather than a blanket rule. Unlike `objectstack://objects` it + // publishes no count and makes no completeness claim to any client: what a + // degraded read costs here is that skills a loader could not be reached for + // are silently not registered as prompts. There is nobody to tell — the + // reply this read shapes is the SDK's own `prompts/list`, whose shape this + // file does not own, and inventing a placeholder prompt to carry the news + // would put a fabricated entry in a list whose entire purpose is to say + // what exists. + // + // So the verdict goes to the operator, at `warn`: a functional degradation + // (the prompt surface is visibly smaller than it should be), not a + // durability one, which is the level AGENTS.md → "Degradation log levels" + // prescribes. What makes it worth saying at all is the snapshot's LIFETIME: + // the stdio transport takes this list once at bridge time, so an outage + // during boot leaves the prompt surface short until the server is + // restarted, long after the loader heals. The HTTP transport rebuilds per + // request and self-heals — the line says which one the reader is looking at. + let skillListVerdict: { degraded: boolean; errors: string[] } = { degraded: false, errors: [] }; const skillBridge: McpSkillBridge = { - listSkills: async () => (await metadataService.list('skill')) ?? [], + listSkills: async () => { + // The verdict of the READ, recorded as the read happens. `listSkills` + // is also the per-call re-read behind each registered prompt's body, so + // this is deliberately last-read-wins rather than boot-only: the + // snapshot check below runs immediately after its own call. + const read = await diagnosedList(metadataService, 'skill'); + skillListVerdict = { degraded: read.degraded, errors: read.errors }; + return read.items; + }, }; let skills: Awaited>; @@ -764,6 +962,23 @@ export class MCPServerRuntime { return; } + // [#6504] The read ANSWERED, and the answer is known-short. Before this the + // two outcomes were indistinguishable from here: `list('skill')` resolves + // an array whether every loader answered or one of them was down, so a + // partial prompt surface was bridged with the same single `info` line a + // complete one gets. + if (skillListVerdict.degraded) { + logger?.warn( + '[MCP] skill prompt list is INCOMPLETE — the metadata service could not be fully read, so skills ' + + 'held by the unreadable loader(s) are missing from this surface. They are missing, NOT undeclared: ' + + 'an MCP client listing prompts now sees fewer than this environment declares. ' + + 'The stdio transport takes this list once at bridge time, so it stays short until the server is ' + + 'restarted, even after the loader recovers; the HTTP transport rebuilds per request and self-heals. ' + + 'Fix: check the loaders behind the metadata service (datasource connection, credentials, table).', + { readable: skills.length, errors: skillListVerdict.errors }, + ); + } + let bridged = 0; for (const skill of skills) { if (skill.name === 'agent_prompt') { diff --git a/packages/metadata/src/metadata-manager-list-diagnosed.test.ts b/packages/metadata/src/metadata-manager-list-diagnosed.test.ts new file mode 100644 index 0000000000..47754c6537 --- /dev/null +++ b/packages/metadata/src/metadata-manager-list-diagnosed.test.ts @@ -0,0 +1,337 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6504 — a KNOWN-PARTIAL `list()` answer must be reachable AS known-partial, + * not only as a shorter array. + * + * --------------------------------------------------------------------------- + * The defect + * --------------------------------------------------------------------------- + * #5184 taught `readListUncached()` to compute `degraded` — did assembling this + * set lose a loader? — and #5253 taught the read to be single-flight. Both kept + * the verdict entirely inside the manager: `list()` spent it on a cache TTL and + * returned `items` alone, and `IMetadataService` declared no plural counterpart + * to `getDiagnosed`. So a consumer receiving a short list could not ask whether + * it was short because that is all anyone declared, or because a loader was + * down. + * + * That is #5840's shape one read over, and sharper there than here: `list` is + * the read whose answer carries a **count**, and a count is the strongest + * positive claim a read can make. `reportLoaderReadFailure`'s own message + * already says what it costs — "every list served from now on is a PARTIAL set + * presented as a complete one, and the server keeps reporting healthy" — but + * until `listDiagnosed` existed that sentence was addressed to a log reader + * only, because no caller had a way to ask. + * + * --------------------------------------------------------------------------- + * Why the failure is REAL and not stubbed + * --------------------------------------------------------------------------- + * Every degraded case below is produced by a `DatabaseLoader` over a driver + * whose `find()` throws `ECONNRESET` — the same harness #5184 uses — so the + * `catch` in `readListUncached()` is the thing under test and `degraded` is + * computed rather than injected. A test that handed the manager a pre-made + * verdict would prove only that a boolean can be passed along, which is exactly + * the property that was never in doubt. + * + * --------------------------------------------------------------------------- + * What is asserted, and why it is the COUNT + * --------------------------------------------------------------------------- + * The load-bearing case is `the outage and the small environment are BYTE-EQUAL + * through list()`: two managers, one whose single loader is down and one that + * genuinely holds nothing more, produce `list()` answers that are deep-equal + * *and equal in length*, while `listDiagnosed()` separates them. A test that + * only checked "a `degraded` flag exists" would pass on an implementation that + * reports the flag against the wrong read; pinning the equality is what makes + * the flag mean something. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Ordinary red. Reversion is defined as deleting `listDiagnosed()` and letting + * `list()` narrow the read itself — i.e. the pre-#6504 producer. + * + * Predicted, written down before running: **9 red / 1 green**. Every case that + * calls `listDiagnosed` goes red (vitest transpiles rather than type-checks, so + * the missing member surfaces as a runtime `TypeError`, not a compile error), + * and exactly ONE case — *"list() alone cannot tell them apart"* — stays green. + * + * That one is an invariant pin rather than a gap, and it is green in BOTH + * directions on purpose: `list()`'s answer is deliberately unchanged by this + * PR, which is what keeps every existing caller working. A case that went red + * there would be reporting a regression, not this fix. The measured result is + * recorded in the PR body as it came out. + */ + +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 outage cases would otherwise print one `error` line +// each, and one case reads 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, +})); + +/** The degraded TTL, read off the class so this file cannot drift from the policy. */ +const degradedTtl = (): number => + (MetadataManager as unknown as { DEGRADED_LIST_CACHE_TTL_MS: number }).DEGRADED_LIST_CACHE_TTL_MS; + +const connectionReset = (): Error => + Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + +/** A named metadata row, as both the registry and the loader hand them back. */ +interface NamedItem { + name: string; +} + +const names = (items: unknown[]): string[] => + (items as NamedItem[]).map((i) => i.name).sort(); + +/** + * A `sys_metadata` store that fails every read until `heal()`, then serves two + * `permission` rows. Minimal on purpose: `loadMany()` only reaches `syncSchema` + * and `find`. + * + * Two rows rather than #5184's one so the outage is a real subtraction from a + * countable total — the healthy answer is 3, the degraded answer is 1, and the + * gap is the thing a `totalCount` consumer would have understated. + */ +function healableStore(): { driver: IDataDriver; heal: () => void } { + let broken = true; + const find = vi.fn(async (): Promise[]> => { + if (broken) throw connectionReset(); + return [ + { id: 'r1', name: 'from_db_a', type: 'permission', metadata: JSON.stringify({ name: 'from_db_a' }) }, + { id: 'r2', name: 'from_db_b', type: 'permission', metadata: JSON.stringify({ name: 'from_db_b' }) }, + ]; + }); + const driver = { + name: 'mock', + version: '1.0.0', + supports: {}, + connect: async (): Promise => {}, + disconnect: async (): Promise => {}, + syncSchema: async (): Promise => {}, + find, + } as unknown as IDataDriver; + + return { + driver, + heal: (): void => { + broken = false; + }, + }; +} + +/** Registry item + a `DatabaseLoader` over a store that is down. */ +function managerOverBrokenStore(): { manager: MetadataManager; heal: () => void } { + const store = healableStore(); + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + // `cache: { enabled: false }` keeps the loader's OWN LRU out of the picture. + manager.registerLoader(new DatabaseLoader({ driver: store.driver, cache: { enabled: false } })); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + return { manager, heal: store.heal }; +} + +/** + * A manager that is genuinely small: one declaration, every loader answering. + * Its `list('permission')` is the answer the broken one above IMITATES. + */ +function managerOverHealthyStore(): MetadataManager { + const manager = new MetadataManager({ formats: ['json'], loaders: [new MemoryLoader()] }); + manager.registerInMemory('permission', 'from_code', { name: 'from_code' }); + return manager; +} + +beforeEach(() => { + logger.error.mockClear(); + logger.info.mockClear(); + logger.warn.mockClear(); + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('#6504 — the fact the defect was: an outage and a small environment are the same list()', () => { + it('list() alone cannot tell them apart — same items, same COUNT', async () => { + const { manager: degraded } = managerOverBrokenStore(); + const healthy = managerOverHealthyStore(); + + const fromOutage = await degraded.list('permission'); + const fromSmallEnvironment = await healthy.list('permission'); + + // Byte-equal through `list()`. This is the defect stated as an + // assertion, and it is deliberately still TRUE after the fix: `list()` + // is unchanged, which is what keeps every existing caller working. + expect(fromOutage).toEqual(fromSmallEnvironment); + expect(fromOutage).toHaveLength(fromSmallEnvironment.length); + expect(names(fromOutage)).toEqual(['from_code']); + + // A consumer rendering the length as a total therefore states the same + // number in both cases — and in one of them it is wrong. + expect(fromOutage.length).toBe(1); + }); + + it('listDiagnosed() separates them, and the count claim is what it withholds', async () => { + const { manager: degraded } = managerOverBrokenStore(); + const healthy = managerOverHealthyStore(); + + const outage = await degraded.listDiagnosed('permission'); + const small = await healthy.listDiagnosed('permission'); + + // The one thing `list()` could not say. + expect(outage.degraded).toBe(true); + expect(small.degraded).toBe(false); + + // Same items and the same count as each other — so the verdict, not the + // payload, is the entire difference. A fix that changed what is served + // would be a functional regression wearing a diagnosis fix's clothes. + expect(outage.items).toEqual(small.items); + expect(outage.items).toHaveLength(small.items.length); + + // And the count that WOULD have been claimed is knowably a floor, not a + // total: the environment declares at least 1 permission, and in fact 3. + expect(outage.items).toHaveLength(1); + }); + + it('names which loader was lost, in `loadDiagnosed`\'s format', async () => { + const { manager } = managerOverBrokenStore(); + + const outage = await manager.listDiagnosed('permission'); + + expect(outage.errors).toHaveLength(1); + expect(outage.errors[0]).toMatch(/^database: /); + expect(outage.errors[0]).toMatch(/ECONNRESET/); + + // A complete read carries no messages at all — `errors` is empty, never + // a stale set left over from a previous outage. + const healthy = await managerOverHealthyStore().listDiagnosed('permission'); + expect(healthy.errors).toEqual([]); + }); +}); + +describe('#6504 — the healthy direction: a complete read signals nothing degraded', () => { + it('a reachable loader set yields the FULL count with degraded false', async () => { + const { manager, heal } = managerOverBrokenStore(); + heal(); + + const read = await manager.listDiagnosed('permission'); + + // All three: the registry item plus both rows the store holds. + expect(names(read.items)).toEqual(['from_code', 'from_db_a', 'from_db_b']); + expect(read.items).toHaveLength(3); + expect(read.degraded).toBe(false); + expect(read.errors).toEqual([]); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('the outage is a MEASURABLE subtraction from that count, not a different shape', async () => { + const { manager: broken } = managerOverBrokenStore(); + const { manager: working, heal } = managerOverBrokenStore(); + heal(); + + const outage = await broken.listDiagnosed('permission'); + const complete = await working.listDiagnosed('permission'); + + // The gap a `totalCount` consumer would have reported as fact: 1 vs 3. + expect(outage.items.length).toBeLessThan(complete.items.length); + expect(outage.items).toHaveLength(1); + expect(complete.items).toHaveLength(3); + }); +}); + +describe('#6504 — list() and listDiagnosed() are one read seen at two widths', () => { + it('agree on the items in both directions, and hand back the SAME array instance', async () => { + const { manager: broken } = managerOverBrokenStore(); + const { manager: working, heal } = managerOverBrokenStore(); + heal(); + + // Degraded: same instance, so the two members cannot drift. + const brokenItems = await broken.list('permission'); + expect((await broken.listDiagnosed('permission')).items).toBe(brokenItems); + + // Healthy: likewise. + const workingItems = await working.list('permission'); + expect((await working.listDiagnosed('permission')).items).toBe(workingItems); + }); + + it('asking for the verdict costs no extra loader walk — one cache entry serves both', 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'); + + await manager.list('permission'); + expect(loadMany).toHaveBeenCalledTimes(1); + + // Served from the entry the `list()` above filled — `listDiagnosed` is + // the same read, not a second one. + const diagnosed = await manager.listDiagnosed('permission'); + expect(loadMany).toHaveBeenCalledTimes(1); + expect(names(diagnosed.items)).toEqual(['stored']); + expect(diagnosed.degraded).toBe(false); + }); + + it('a memoized DEGRADED read serves its verdict too, not just its items', async () => { + const { manager } = managerOverBrokenStore(); + + await manager.list('permission'); + expect(logger.error).toHaveBeenCalledTimes(1); + + // Inside the degraded window: answered from cache, still known-partial. + // Before #6504 the cache was the only place this fact lived. + const cached = await manager.listDiagnosed('permission'); + expect(cached.degraded).toBe(true); + expect(cached.errors).toHaveLength(1); + // Still one line — reading the verdict must not re-walk the loaders and + // re-report the outage. + expect(logger.error).toHaveBeenCalledTimes(1); + }); + + it('a listDiagnosed caller JOINING an in-flight read receives that read\'s verdict', async () => { + const { manager } = managerOverBrokenStore(); + + // Issued together, so the second joins the first rather than starting + // its own walk (#5253). The joiner must not lose the verdict the read + // it joined had already computed. + const [plain, diagnosed] = await Promise.all([ + manager.list('permission'), + manager.listDiagnosed('permission'), + ]); + + expect(diagnosed.degraded).toBe(true); + expect(diagnosed.items).toBe(plain); + // One read, so one outage line — not one per caller. + expect(logger.error).toHaveBeenCalledTimes(1); + }); +}); + +describe('#6504 — recovery is reported through the verdict, not only the log', () => { + it('flips back to degraded:false once the store heals and the short TTL lapses', async () => { + const { manager, heal } = managerOverBrokenStore(); + + const duringOutage = await manager.listDiagnosed('permission'); + expect(duringOutage.degraded).toBe(true); + expect(duringOutage.items).toHaveLength(1); + + heal(); + vi.advanceTimersByTime(degradedTtl() + 1); + + const afterRecovery = await manager.listDiagnosed('permission'); + expect(afterRecovery.degraded).toBe(false); + expect(afterRecovery.errors).toEqual([]); + // The count a consumer may now state as a total. + expect(afterRecovery.items).toHaveLength(3); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index e47ee9e557..946df8861a 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -255,6 +255,35 @@ interface ListCacheEntry { * this rather than having to guess. */ degraded: boolean; + /** + * [#6504] The messages of the loaders that could not be read, in the order + * they failed — empty whenever `degraded` is false. + * + * Memoized with the entry rather than recomputed, so a consumer served from + * the cache learns the same thing as one served by the read that filled it. + * Without this the cache could say *that* the answer is partial but never + * *why*, which is the half `listDiagnosed` needs to be as informative as + * `getDiagnosed` already is. + */ + errors: string[]; +} + +/** + * [#6504] What one `list()` read actually produced: the best-effort set, plus + * whether assembling it lost a loader. + * + * The return of {@link MetadataManager.readListUncached}, the value shared + * through `inflightListReads`, and — minus the cache bookkeeping — what + * {@link MetadataManager.listDiagnosed} hands to a caller. One shape for all + * three on purpose: the verdict used to be dropped at each hop outward + * (`readListUncached` computed it, the in-flight promise kept only `items`, + * `list()` returned only that), and a single carried record is what makes + * losing it again take an edit rather than an omission. + */ +interface ListReadResult { + items: unknown[]; + degraded: boolean; + errors: string[]; } export interface MetadataManagerOptions extends MetadataManagerConfig { @@ -410,8 +439,17 @@ export class MetadataManager implements IMetadataService { * only, so a fresh read that already replaced it keeps its slot. Nothing * accumulates — a wave of callers arriving after settle finds the cache the * settle just wrote, and once that lapses it starts one new read. + * + * [#6504] The shared value is the whole {@link ListReadResult}, not just + * `items`. "Sharers share the outcome" above is stated about the answer *and* + * its degraded verdict, and while the promise carried only `items` that was + * true of `list()` alone: a {@link listDiagnosed} caller joining an in-flight + * read had no way to reach the verdict that read had already computed, and + * would have had to either re-walk the loaders (defeating this map) or invent + * a second, unmemoized answer. `list()` narrows to `.items` at its own return + * instead, so every sharer still receives the same array instance. */ - private readonly inflightListReads = new Map>(); + private readonly inflightListReads = new Map>(); // [#5108] Loader names whose read failure has already been reported at // `error` by `list()`. AGENTS.md → "Degradation log levels": say it once, at @@ -880,12 +918,59 @@ export class MetadataManager implements IMetadataService { * `listCache`. */ async list(type: string): Promise { + return (await this.readList(type)).items; + } + + /** + * `list`, plus whether the answer can be trusted as complete. + * + * [#6504] The plural counterpart of {@link getDiagnosed}, and the same defect + * one read over: `readListUncached` has computed this verdict since #5184 and + * `list()` spent it entirely on a cache TTL, so a consumer receiving a short + * set could not ask whether it was short because that is all anyone declared + * or because a loader was down. {@link reportLoaderReadFailure}'s own message + * says what that costs — "every list served from now on is a PARTIAL set + * presented as a complete one, and the server keeps reporting healthy" — and + * until this member existed that sentence was addressed to a log reader only, + * because no caller had a way to ask. + * + * Sharper than the singular case rather than merely analogous: `list` is the + * read whose answer carries a **count**, and a consumer restating + * `items.length` as "this environment contains N items" makes a positive, + * numeric claim about what an author declared out of a read that partly did + * not happen. + * + * Reads through exactly the same cache and single-flight machinery `list()` + * does — same entry, same TTLs, same in-flight join — so asking for the + * verdict costs no extra loader walk, and `list()` and + * `listDiagnosed().items` cannot drift: they are the same read, narrowed at + * different points. `degraded` is true when at least one loader threw while + * this set was assembled; unlike {@link getDiagnosed} it does NOT additionally + * require that nothing answered, because a plural read that lost one loader is + * partial even when the others answered plenty — which is the whole fact. + */ + async listDiagnosed(type: string): Promise { + const { items, degraded, errors } = await this.readList(type); + return { items, degraded, errors }; + } + + /** + * The cached / single-flight read behind {@link list} and + * {@link listDiagnosed}. + * + * [#6504] Extracted so the two members are one read seen at two widths rather + * than two implementations that have to be kept in agreement — the shape + * `get`/`getDiagnosed` pay for with a duplicated body and a test pinning them + * to each other. Everything below is unchanged in behaviour from when it was + * inlined in `list()`; only the verdict now survives the return. + */ + private async readList(type: string): Promise { // 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; + return cached; } // [#5253] Cold cache, but not necessarily a cold read: join the walk @@ -898,7 +983,7 @@ export class MetadataManager implements IMetadataService { // Registering the read is what permits it to memoize its own result — // `invalidateListCache()` retracts the registration, and both the cache // write and the cleanup below act only while the slot is still ours. - const shared: Promise = this.readListUncached(type).then(({ items, degraded }) => { + const shared: Promise = this.readListUncached(type).then((result) => { // [#5184] The degraded verdict of a SHARED read is the degraded verdict // of the read: sharing must not become a back door that lands a // known-partial answer on the 30s healthy TTL. Every sharer received @@ -906,9 +991,9 @@ export class MetadataManager implements IMetadataService { // [#5253] Skipped when an invalidation crossed this read, so a write // that landed mid-read is never re-buried under the pre-write answer. if (this.inflightListReads.get(type) === shared) { - this.cacheListResult(type, items, degraded); + this.cacheListResult(type, result); } - return items; + return result; }); this.inflightListReads.set(type, shared); @@ -935,7 +1020,7 @@ export class MetadataManager implements IMetadataService { * result may be memoized depends on what happened to the read's registration * while it ran, which only `list()` can see. */ - private async readListUncached(type: string): Promise<{ items: unknown[]; degraded: boolean }> { + private async readListUncached(type: string): Promise { const items = new Map(); // From in-memory registry @@ -950,7 +1035,14 @@ export class MetadataManager implements IMetadataService { // 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. + // [#6504] `errors` records WHICH loaders were lost and why. The messages + // were already being produced here and handed only to the logger — which + // speaks once per outage episode, so a consumer arriving mid-outage finds + // nothing to read. Collected per read (not once per episode) because this + // describes THIS answer, and it is what {@link listDiagnosed} reports + // alongside the set. let degraded = false; + const errors: string[] = []; for (const loader of this.loaders.values()) { try { const loaderItems = await loader.loadMany(type); @@ -963,11 +1055,12 @@ export class MetadataManager implements IMetadataService { this.reportLoaderReadRecovered(loader.contract.name); } catch (e) { degraded = true; + errors.push(`${loader.contract.name}: ${e instanceof Error ? e.message : String(e)}`); this.reportLoaderReadFailure(loader.contract.name, type, e); } } - return { items: Array.from(items.values()), degraded }; + return { items: Array.from(items.values()), degraded, errors }; } /** @@ -1039,9 +1132,14 @@ export class MetadataManager implements IMetadataService { * 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. + * + * [#6504] Takes the whole read result rather than its parts for the same + * reason: a signature that spreads the verdict across positional arguments is + * one a later caller can quietly fill with `false`, which is how the verdict + * was lost on the way out in the first place. */ - private cacheListResult(type: string, items: unknown[], degraded: boolean): void { - this.listCache.set(type, { ts: Date.now(), items, degraded }); + private cacheListResult(type: string, result: ListReadResult): void { + this.listCache.set(type, { ts: Date.now(), ...result }); } /** diff --git a/packages/spec/src/contracts/metadata-service.ts b/packages/spec/src/contracts/metadata-service.ts index 3b94821a8b..b8aa1ba504 100644 --- a/packages/spec/src/contracts/metadata-service.ts +++ b/packages/spec/src/contracts/metadata-service.ts @@ -329,11 +329,55 @@ export interface IMetadataService { /** * List all metadata items of a given type + * + * The returned array is AMBIGUOUS by construction, in the same way + * {@link get}'s `undefined` is: a short list means "that is all anyone + * declared" *and* "a loader that could hold more of them failed". Prefer + * {@link listDiagnosed} wherever the difference could change a decision, and + * especially before restating the array's LENGTH as a fact about the + * environment (#6504). + * * @param type - Metadata type * @returns Array of metadata definitions */ list(type: string): Promise; + /** + * List all metadata items of a given type, and say whether the answer can + * be trusted as complete. The plural counterpart of {@link getDiagnosed}, + * drawing the same ADR-0110 D3 distinction on the read that returns a set. + * + * [#6504] Declared for the reason {@link getDiagnosed} was, with one thing + * added that makes it sharper rather than merely analogous. On the singular + * read a MISS and an OUTAGE arrive as the same `undefined`; on the plural + * read they arrive as the same *array*, and an array carries a **count**. + * A count is the strongest positive claim a read can make — a consumer that + * renders `list(t).length` as "this environment contains N items" states, + * positively and numerically, something no read established. `list` cannot + * be fixed in place (every caller would have to change at once, and most + * are right to keep ignoring the verdict), so the verdict is offered + * alongside it, exactly as `getDiagnosed` is offered alongside `get`. + * + * `degraded` is true when at least one loader could not be read while the + * set was assembled, with those loaders' messages in `errors`. It says the + * set is known-PARTIAL — never that it is empty, and never that it is + * wrong: `items` is still the best-effort answer and is served as it always + * was. A consumer must not read `degraded` as a reason to withhold the + * items; it is a reason to withhold any claim of COMPLETENESS made on top + * of them. + * + * Optional, for the same reason its singular twin is: implementations that + * predate it simply cannot report the distinction, so a consumer that + * probes for it must keep reading {@link list} when it is absent — and a + * service without it reports nothing degraded, which is precisely what it + * could express. + * + * @param type - Metadata type + */ + listDiagnosed?( + type: string, + ): Promise<{ items: unknown[]; degraded: boolean; errors: string[] }>; + /** * Unregister/remove a metadata item by type and name. * @@ -412,6 +456,15 @@ export interface IMetadataService { /** * Convenience: list all object definitions + * + * Carries the same ambiguity {@link list} documents — a short answer may be + * an outage rather than a small environment (#6504). This member declares + * no equivalence to `list('object')`, so it has no diagnosed twin of its + * own: a consumer that needs the verdict asks {@link listDiagnosed} for it + * separately rather than presuming the two reads resolve the same set. That + * is the plural instance of the fork {@link getObject} records, and it is + * deliberately left as a fork rather than settled here. + * * @returns Array of object definitions */ listObjects(): Promise;