diff --git a/.changeset/list-diagnosed-consumer-sweep.md b/.changeset/list-diagnosed-consumer-sweep.md new file mode 100644 index 0000000000..5dbd30920d --- /dev/null +++ b/.changeset/list-diagnosed-consumer-sweep.md @@ -0,0 +1,56 @@ +--- +"@objectstack/service-datasource": minor +"@objectstack/runtime": minor +"@objectstack/mcp": minor +--- + +fix(runtime,mcp,service-datasource): the #6504 consumer sweep — three list consumers stop making claims a known-partial read cannot support (#6504) + + + +`IMetadataService.listDiagnosed?(type)` (PR #7721) lets a plural read say whether +its answer can be trusted as complete. This is the consumer half: the callers +that were restating a possibly-short listing as a fact about the environment. + +Each consumer was qualified individually, per PR #6051's discipline, and most +were left alone — a caller publishing a snapshot with no count has nothing to +mis-state. Three make a claim, and each now withholds exactly that claim while +still serving everything it could read: + +- **`removeDatasource` no longer deletes on a bound-object count it could not + take completely.** The guard `if (bound > 0) throw` is the only thing standing + in front of an irreversible delete that also unbinds the datasource's secret, + and its input is derived from the metadata service's object listing. During a + loader outage that listing goes silently short, and the worst value is the + benign one: `0` reads exactly like "nothing is bound", so the guard OPENED. + It now refuses with `SERVICE_UNAVAILABLE` / 503 — a dependency outage the + operator can retry, not a client error — and the record, its credential and + its pool all survive. +- **The MCP `list_objects` tool stops publishing `totalCount` on a known-partial + listing.** This is the same claim PR #7721 removed from the + `objectstack://objects` resource, on the other MCP primitive: same payload + shape, different door, never covered. A degraded read now serves the same + objects with `totalCount` **absent** and `partial` / `returnedCount` / + `warning` plus the 503 envelope in its place, so a client reading the total + gets `undefined` rather than a believable wrong integer. Both bridges + implement it — stdio (`@objectstack/mcp`) and HTTP (`@objectstack/runtime`) — + because a completeness claim must not depend on which transport a client + connected over. +- **The ADR-0015 §5.2 boot gate stops announcing an all-clear over a sweep it + could not complete.** It validated whatever `listObjects()` returned and then + logged *all federated objects match their remote schema*, with a count. + Federated objects behind an unreadable loader were never validated, so + `onMismatch: 'fail'` could not have fired for them. The gate now warns that + the swept set was incomplete and names what it did validate. ⛔ It does **not** + abort boot on a degraded metadata read: turning a transient outage into a + refusal to start would be a new failure mode bought with a diagnosis fix. + +Every new member is optional in the same way `listDiagnosed` itself is: a host +whose metadata service predates the verdict behaves exactly as it did before, +and a service without it reports nothing degraded — precisely what it could +express. diff --git a/packages/mcp/src/mcp-http-tools.list-objects-outage.test.ts b/packages/mcp/src/mcp-http-tools.list-objects-outage.test.ts new file mode 100644 index 0000000000..11c1f06c55 --- /dev/null +++ b/packages/mcp/src/mcp-http-tools.list-objects-outage.test.ts @@ -0,0 +1,245 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6504 (consumer sweep) — the `list_objects` TOOL publishes `totalCount`, and + * a count taken over a known-partial listing is the strongest false claim a + * read can make. + * + * --------------------------------------------------------------------------- + * Why this surface, when PR #7721 already closed its sibling + * --------------------------------------------------------------------------- + * PR #7721 fixed `objectstack://objects` — the RESOURCE — because it rendered + * `{ objects, totalCount }` and told an MCP client, with a number, that the + * environment contained fewer objects than it does. The `list_objects` TOOL + * renders the SAME payload from the same underlying listing and was not + * covered: the resource is served by `MCPServerRuntime` off `IMetadataService` + * directly, while the tool is served through the injected `McpDataBridge` (the + * stdio bridge in this package, the HTTP bridge in `packages/runtime`), so the + * two paths never met. A client asking "how many objects does this app have?" + * therefore got an honest answer over one door and a confident wrong integer + * over the other, depending on which primitive it happened to use. + * + * The fix is the resource's, in the resource's words: withhold the CLAIM, not + * the data. Healthy stays byte-identical; degraded serves the same objects with + * `totalCount` ABSENT and `partial` / `returnedCount` / `warning` plus the 503 + * envelope in its place. A client reading `totalCount` then gets `undefined` — + * which fails, or renders as nothing — where a plausible integer would have + * been believed. + * + * --------------------------------------------------------------------------- + * DOUBLES HERE, and where the real loader failure is pinned instead + * --------------------------------------------------------------------------- + * `packages/mcp` does not depend on `@objectstack/metadata` — adding it for a + * test would be a larger change than the fix — so the bridge below is a double, + * the same split PR #7721 and #6055 both took and stated rather than papered + * over. The verdict these doubles hand back is the exact shape + * `MetadataManager.listDiagnosed()` returns from a live `ECONNRESET`, pinned + * against a real `DatabaseLoader` in + * `packages/metadata/src/metadata-manager-list-diagnosed.test.ts` and, for this + * sweep's consumer half, in + * `packages/runtime/src/list-diagnosed-consumer-sweep.test.ts` — which drives + * the runtime's implementation of this very bridge member off a real failing + * loader. What is pinned HERE is the only thing that lives here: what the tool + * renders once it holds the verdict. + * + * Everything below drives the REAL MCP HTTP transport (`tools/call`), not the + * handler in isolation, so the payload asserted is the one a client receives. + * + * --------------------------------------------------------------------------- + * Both directions, on the COUNT + * --------------------------------------------------------------------------- + * The load-bearing pair is two answers with the SAME objects and the same + * length — one from an outage, one from a genuinely small environment — where + * only the presence of `totalCount` may differ. A test asserting merely that + * `partial` appears would pass on a build that also kept publishing the wrong + * total beside it, which is the failure this is guarding against, so the + * ABSENCE of the key is asserted explicitly in the degraded direction and its + * presence in the healthy one. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Ordinary red. Reversion is defined as restoring the pre-#6504 tool body — + * `const objects = await bridge.listObjects()` and an unconditional + * `{ objects: visible, totalCount: visible.length }` — leaving + * `listObjectsDiagnosed` declared on the interface and implemented on both + * bridges, but unread. That is the *declared-but-unconsumed* shape, and it is + * the ablation worth taking, because a whole-file revert would also delete the + * interface member and turn the optionality cases red for the wrong reason. + * + * Predicted, written down before running: **3 red / 3 green** of the 6. Red are + * the three cases that discriminate on the withheld claim (the degraded + * payload, the same-count pair, and the system-object filter's + * `returnedCount`). Green are the healthy byte-identical case and both + * optionality cases — a bridge with no diagnosed member takes the same code + * path in either direction, which is exactly what makes the member optional. + * Measured result is recorded in the PR body as it came out. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; + +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import type { McpDataBridge, McpObjectSummary } from './mcp-http-tools.js'; + +const LOADER_FAILURE = 'database: read ECONNRESET'; + +/** The one object that survived the outage, plus a system object for the filter case. */ +const READABLE: McpObjectSummary[] = [ + { name: 'task', label: 'Task', fieldCount: 4 }, + { name: 'sys_user', label: 'User', fieldCount: 9 }, +]; + +type BridgeOpts = { + objects?: McpObjectSummary[]; + /** Omit entirely to model a bridge predating #6504 (the member is optional). */ + diagnosed?: { degraded: boolean; errors: string[] } | 'absent'; +}; + +function makeBridge(opts: BridgeOpts = {}): McpDataBridge { + const objects = opts.objects ?? READABLE; + const bridge: any = { + async listObjects() { return objects; }, + async describeObject(name: string) { return { name }; }, + async query() { return { records: [] }; }, + async get() { return {}; }, + async create() { return {}; }, + async update() { return {}; }, + async remove() { return { success: true }; }, + }; + if (opts.diagnosed !== 'absent') { + const verdict = opts.diagnosed ?? { degraded: false, errors: [] }; + bridge.listObjectsDiagnosed = async () => ({ objects, ...verdict }); + } + return bridge as McpDataBridge; +} + +/** Call `list_objects` over the real transport and hand back its parsed body. */ +async function listObjects( + runtime: MCPServerRuntime, + bridge: McpDataBridge, + toolOptions?: Record, +): Promise { + const body = { + jsonrpc: '2.0', + id: 1, + method: 'tools/call', + params: { name: 'list_objects', arguments: {} }, + }; + const res = await runtime.handleHttpRequest( + new Request('http://localhost/api/v1/mcp', { + method: 'POST', + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, + body: JSON.stringify(body), + }), + { bridge, parsedBody: body, ...(toolOptions ? { toolOptions } : {}) } as any, + ); + const json: any = await res.json(); + expect(json.error, 'precondition: the tool must have answered').toBeUndefined(); + expect(json.result?.isError, 'precondition: the tool must not have errored').not.toBe(true); + return JSON.parse(json.result.content[0].text); +} + +describe('#6504 — list_objects withholds its totalCount on a known-partial listing', () => { + let runtime: MCPServerRuntime; + beforeEach(() => { + runtime = new MCPServerRuntime({ name: 't', version: '1.0.0' }); + }); + + it('healthy: `{ objects, totalCount }`, unchanged — a complete read may state its count', async () => { + const body = await listObjects(runtime, makeBridge({ diagnosed: { degraded: false, errors: [] } })); + + expect(body.totalCount).toBe(1); + expect(body.objects.map((o: any) => o.name)).toEqual(['task']); + // Nothing from the degraded branch leaks into a healthy answer. + expect(body.partial).toBeUndefined(); + expect(body.warning).toBeUndefined(); + expect(body.code).toBeUndefined(); + }); + + it('degraded: the SAME objects, `totalCount` ABSENT, and a structural 503 envelope', async () => { + const body = await listObjects( + runtime, + makeBridge({ diagnosed: { degraded: true, errors: [LOADER_FAILURE] } }), + ); + + // The data is still served — this is a diagnosis fix, not a functional one. + expect(body.objects.map((o: any) => o.name)).toEqual(['task']); + + // The claim, and only the claim, is withheld. `undefined` rather than a + // smaller integer is the entire point: a client reading it fails loudly + // instead of believing a number nobody established. + expect(body.totalCount).toBeUndefined(); + expect('totalCount' in body, 'the key must be ABSENT, not present-and-nullish').toBe(false); + + expect(body.partial).toBe(true); + expect(body.returnedCount).toBe(1); + expect(body.code).toBe('SERVICE_UNAVAILABLE'); + expect(body.status).toBe(503); + expect(body.warning).toMatch(/known to be INCOMPLETE/); + // The sentence names the served count as a FLOOR, never as a total. + expect(body.warning).toMatch(/at least that many objects/); + }); + + it('the outage and the small environment differ ONLY in the claim, never in the data', async () => { + // The pair that gives the verdict meaning: byte-equal object lists, equal + // lengths, opposite entitlement to publish a total. + const outage = await listObjects( + runtime, + makeBridge({ diagnosed: { degraded: true, errors: [LOADER_FAILURE] } }), + ); + const small = await listObjects( + runtime, + makeBridge({ diagnosed: { degraded: false, errors: [] } }), + ); + + expect(outage.objects).toEqual(small.objects); + expect(outage.objects).toHaveLength(small.objects.length); + expect(small.totalCount).toBe(1); + expect(outage.totalCount).toBeUndefined(); + expect(outage.returnedCount).toBe(small.totalCount); + }); + + it('`returnedCount` counts what is SERVED — after the system-object filter, not before', async () => { + // Naming the pre-filter number would restate the same over-claim one field + // along: the client can see two objects and would be told about three. + const body = await listObjects( + runtime, + makeBridge({ + objects: [ + { name: 'task' }, + { name: 'invoice' }, + { name: 'sys_user' }, + ], + diagnosed: { degraded: true, errors: [LOADER_FAILURE] }, + }), + { allowSystemObjects: false }, + ); + + expect(body.objects.map((o: any) => o.name)).toEqual(['task', 'invoice']); + expect(body.returnedCount).toBe(2); + expect(body.warning).toMatch(/2 are being served/); + }); + + it('a bridge PREDATING listObjectsDiagnosed behaves exactly as before', async () => { + // The optionality is the bridge's own graceful-degradation contract, and a + // host that cannot ask its metadata service for a verdict must not have one + // invented for it. + const body = await listObjects(runtime, makeBridge({ diagnosed: 'absent' })); + + expect(body.totalCount).toBe(1); + expect(body.partial).toBeUndefined(); + }); + + it('a bridge predating it does not become "degraded" merely by being old', async () => { + // The direction that matters for a false ALARM: absence of the member is + // "cannot report", never "known-partial". Asserted separately from the case + // above because that one would also pass if the 503 envelope were emitted + // alongside a totalCount. + const body = await listObjects(runtime, makeBridge({ diagnosed: 'absent' })); + + expect(body.code).toBeUndefined(); + expect(body.status).toBeUndefined(); + expect(body.warning).toBeUndefined(); + expect(body.returnedCount).toBeUndefined(); + }); +}); diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index c7a1a83064..24a8099b15 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -46,6 +46,11 @@ import { inferExpressionType, type FieldRole, } from '@objectstack/formula'; +import { + METADATA_UNAVAILABLE_CODE, + metadataPartialListingSentence, + type DiagnosedObjectListing, +} from './metadata-completeness.js'; export interface McpObjectSummary { name: string; @@ -60,6 +65,24 @@ export interface McpObjectSummary { */ export interface McpDataBridge { listObjects(): Promise; + /** + * [#6504] The same listing, plus whether it can be trusted as COMPLETE. + * + * The `list_objects` tool renders its answer as `{ objects, totalCount }`, + * and `totalCount` is a positive, numeric claim about what this environment + * declares. During a metadata loader outage that claim is simply false, and + * nothing in the payload lets a client tell it from a genuinely small + * environment — the ADR-0110 D3 shape the `objectstack://objects` RESOURCE + * already closed (PR #7721) and this TOOL did not. The two are the same + * question asked over two transports, so they now answer it the same way. + * + * OPTIONAL, and its optionality is the bridge's own graceful-degradation + * contract (same as {@link McpDataBridge.aggregate}), stacked on + * `IMetadataService.listDiagnosed`'s: a bridge that cannot ask its metadata + * service for a verdict omits this member, the tool behaves exactly as it did + * before, and nothing anywhere claims completeness it did not establish. + */ + listObjectsDiagnosed?(): Promise>; describeObject(name: string): Promise; query( object: string, @@ -306,11 +329,48 @@ export function registerObjectTools( inputSchema: {}, annotations: { readOnlyHint: true, destructiveHint: false, openWorldHint: false }, }, + // [#6504] This tool MIS-DESCRIBES during a metadata loader outage, which + // is why it changes while most consumers in that sweep correctly do not: + // it publishes `totalCount`, and a count is the strongest positive claim + // a read can make. The fix withholds the CLAIM, not the data — the same + // treatment, in the same words, the `objectstack://objects` resource got + // in PR #7721. + // + // - healthy → `{ objects, totalCount }`, byte-identical to before. A + // count from a complete read is a fact this tool was + // always right to state. + // - degraded → the same `objects` (the reachable set is still the most + // useful true thing here), `totalCount` ABSENT, and in its + // place `partial` / `returnedCount` / `warning` plus the + // 503 envelope so a client can branch structurally. + // + // Dropping the key rather than reporting a smaller number is the point: a + // client reading `totalCount` gets `undefined` — which fails, or renders + // as nothing — where a plausible-looking integer would have been believed. + // + // `returnedCount` counts what this tool actually SERVES, i.e. after the + // system-object filter, not what the bridge handed over. The two differ + // whenever `allowSystemObjects` is false, and naming the pre-filter number + // here would restate the same over-claim one field along. async () => { try { - const objects = await bridge.listObjects(); - const visible = allowSystem ? objects : objects.filter((o) => !isSystemObject(o.name)); - return textResult({ objects: visible, totalCount: visible.length }); + const diagnosed = bridge.listObjectsDiagnosed + ? await bridge.listObjectsDiagnosed() + : { objects: await bridge.listObjects(), degraded: false, errors: [] }; + const visible = allowSystem + ? diagnosed.objects + : diagnosed.objects.filter((o) => !isSystemObject(o.name)); + if (!diagnosed.degraded) { + return textResult({ objects: visible, totalCount: visible.length }); + } + return textResult({ + objects: visible, + partial: true, + returnedCount: visible.length, + warning: metadataPartialListingSentence('objects', visible.length), + code: METADATA_UNAVAILABLE_CODE, + status: 503, + }); } catch (err) { return errorResult(messageOf(err)); } diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index 85b2f85b03..e9cb5269d7 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -13,6 +13,10 @@ import type { RegisterObjectToolsOptions, RegisterActionToolsOptions, } from './mcp-http-tools.js'; +import { + METADATA_UNAVAILABLE_CODE, + metadataPartialListingSentence, +} from './metadata-completeness.js'; import { protocolStdout } from './protocol-stdout.js'; import { renderSkillMarkdown, type RenderSkillOptions } from './skill-md.js'; import { @@ -72,21 +76,19 @@ const DESTRUCTIVE_TOOLS = new Set([ // ── Metadata outage vs. metadata miss (#6055, ADR-0110 D3) ─────────────────── /** - * [#6055] The classification this file gives "the metadata read did not - * happen", and the classification it gives "the read happened and found - * nothing". Both are the standard catalog's own codes for their status - * (`HttpStatusErrorCodeMap[503]` / `[404]`, ADR-0112) — the same spelling the + * [#6055] The classification this file gives "the read happened and found + * nothing", as opposed to {@link METADATA_UNAVAILABLE_CODE} for "the read did + * not happen". Both are the standard catalog's own codes for their status + * (`HttpStatusErrorCodeMap[404]` / `[503]`, ADR-0112) — the same spelling the * `sys_metadata` half of this family already emits (#5532 / #5843 / #5705), not * a vocabulary invented for MCP. * - * There is no HTTP status on this surface: MCP answers `prompts/get` with a - * `GetPromptResult` and `resources/read` with a `ReadResourceResult`, and - * neither carries an error envelope (only `CallToolResult` has `isError`). So - * the code travels in the payload the surface already had — text for a prompt, - * the JSON body for a resource — and that is the strongest discriminator this - * transport offers. See the PR body for why the channel was not changed. + * [#6504] Its 503 twin moved to `./metadata-completeness.js` when the + * `list_objects` TOOL joined the `objectstack://objects` RESOURCE in withholding + * the same claim: two surfaces answering one question must say it in one + * vocabulary, and this file cannot export to `mcp-http-tools.ts` (it imports + * from it). */ -const METADATA_UNAVAILABLE_CODE = 'SERVICE_UNAVAILABLE'; const METADATA_MISS_CODE = 'RESOURCE_NOT_FOUND'; /** @@ -115,26 +117,15 @@ 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. + * [#6504] `metadataPartialListingSentence` — 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, while the plural one serves the best-effort set and withholds + * only the completeness claim on top of it. * - * 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. + * It now lives in `./metadata-completeness.js` with the 503 code it travels + * with — see the note on {@link METADATA_MISS_CODE}. */ -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 { diff --git a/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts b/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts index 3c576f4f02..f7061cc336 100644 --- a/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts +++ b/packages/mcp/src/mcp-write-response-internal-fields.tripwire.test.ts @@ -133,6 +133,11 @@ const RECIPES: Record = { // ── read / summary faces: no engine write result to strip. Enumerated so the // map stays total and a rename is noticed. ───────────────────────────── listObjects: { invoke: (b) => b.listObjects(), writesRecords: false }, + // [#6504] `listObjects` seen at its second width — the same object summaries + // plus the completeness verdict. A summary face like its twin: it echoes no + // engine write result, and the `{ name, label, fieldCount }` projection both + // share is what keeps a stored field off this response in the first place. + listObjectsDiagnosed: { invoke: (b) => b.listObjectsDiagnosed(), writesRecords: false }, describeObject: { invoke: (b) => b.describeObject('vault'), writesRecords: false }, query: { invoke: (b) => b.query('vault', {}), writesRecords: false }, get: { invoke: (b) => b.get('vault', 'row-1'), writesRecords: false }, diff --git a/packages/mcp/src/metadata-completeness.ts b/packages/mcp/src/metadata-completeness.ts new file mode 100644 index 0000000000..d07c2cb1fb --- /dev/null +++ b/packages/mcp/src/metadata-completeness.ts @@ -0,0 +1,98 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * metadata-completeness — the shared vocabulary for "this MCP surface is + * serving a listing it knows to be SHORT" (#6504, ADR-0110 D3). + * + * Extracted from `mcp-server-runtime.ts` rather than copied. Two surfaces in + * this package now withhold a completeness claim on the same verdict — the + * `objectstack://objects` RESOURCE and the `list_objects` TOOL — and they must + * say the same thing in the same words, or a client that branches on one + * learns nothing about the other. `mcp-server-runtime.ts` imports + * `mcp-http-tools.ts`, so this lives beside them both instead of being + * exported from either (an import back would be a cycle). + */ + +/** + * [#6055] The classification this package gives "the metadata read did not + * happen" — as opposed to "the read happened and found nothing" + * (`RESOURCE_NOT_FOUND`, which stays private to the runtime file that uses it). + * The standard catalog's own code for its status (`HttpStatusErrorCodeMap[503]`, + * ADR-0112) — the same spelling the `sys_metadata` half of this family already + * emits (#5532 / #5843 / #5705), not a vocabulary invented for MCP. + * + * There is no HTTP status on the resource/prompt surfaces: MCP answers + * `prompts/get` with a `GetPromptResult` and `resources/read` with a + * `ReadResourceResult`, and neither carries an error envelope (only + * `CallToolResult` has `isError`). So the code travels in the payload the + * surface already had — text for a prompt, the JSON body for a resource — and + * that is the strongest discriminator this transport offers. + */ +export const METADATA_UNAVAILABLE_CODE = 'SERVICE_UNAVAILABLE'; + +/** + * [#6504] The sentence for "a listing that is known to be SHORT". + * + * The best-effort set IS served: a partial listing is still the most useful + * true thing these surfaces have, 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. + */ +export 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.' + ); +} + +/** + * [#6504] What a bridge reports when it can say whether its object listing was + * complete — the shape of `McpDataBridge.listObjectsDiagnosed`. + * + * `degraded` means the set is known-PARTIAL: never that it is empty, and never + * that it is wrong. `objects` is still the best-effort answer and is served as + * it always was. + */ +export interface DiagnosedObjectListing { + objects: T[]; + degraded: boolean; + errors: string[]; +} + +/** + * [#6504] Ask a metadata service whether its object listing can be trusted as + * complete, without re-resolving the listing through it. + * + * The composition PR #7721 established and this sweep reuses at every consumer: + * the ITEMS come from whatever resolver the call site already used + * (`listObjects()`), and only the VERDICT is asked of `listDiagnosed`, the + * member declared to answer it. `listObjects` is its own member of + * `IMetadataService` and declares no equivalence to `list('object')`, so + * resolving the items through the diagnosed read instead would presume one — + * the private dialect Prime Directive #12 forbids. On the implementation that + * ships they are the same read and share one cache entry and one single-flight + * slot, so the probe costs nothing; where they differ the verdict describes the + * loader set, which can only WITHHOLD a completeness claim, never manufacture + * one. + * + * A service predating `listDiagnosed` reports nothing degraded — precisely what + * it could express. + */ +export async function diagnoseObjectListRead( + metadataService: { + listDiagnosed?: (type: string) => Promise<{ degraded?: boolean; errors?: unknown } | undefined>; + } | undefined | null, +): Promise<{ degraded: boolean; errors: string[] }> { + if (typeof metadataService?.listDiagnosed !== 'function') { + return { degraded: false, errors: [] }; + } + const diagnosed = await metadataService.listDiagnosed('object'); + return { + degraded: diagnosed?.degraded === true, + errors: Array.isArray(diagnosed?.errors) ? (diagnosed.errors as string[]) : [], + }; +} diff --git a/packages/mcp/src/stdio-data-bridge.ts b/packages/mcp/src/stdio-data-bridge.ts index 9d4f49c497..7d625c34bd 100644 --- a/packages/mcp/src/stdio-data-bridge.ts +++ b/packages/mcp/src/stdio-data-bridge.ts @@ -86,6 +86,10 @@ import type { IDataEngine, IMetadataService } from '@objectstack/spec/contracts' // there is no reason to add a second import path to the same function. import { recordNotFoundError } from '@objectstack/core'; import type { McpDataBridge, McpObjectSummary } from './mcp-http-tools.js'; +import { + diagnoseObjectListRead, + type DiagnosedObjectListing, +} from './metadata-completeness.js'; /** What {@link createStdioDataBridge} needs from the host plugin. */ export interface StdioDataBridgeDeps { @@ -296,6 +300,22 @@ export function createStdioDataBridge(deps: StdioDataBridgeDeps): McpDataBridge })); }, + /** + * [#6504] `listObjects` with the completeness verdict attached, so the + * `list_objects` tool can withhold `totalCount` on a known-partial read. + * + * The resolver above is reused rather than re-implemented: the items are + * whatever `listObjects()` answers, and only the verdict is asked of + * `listDiagnosed('object')` — see {@link diagnoseObjectListRead} for why + * this composition, and not resolving the items through the diagnosed read, + * is the correct one. + */ + async listObjectsDiagnosed(): Promise> { + const objects = await bridge.listObjects(); + const { degraded, errors } = await diagnoseObjectListRead(metadataService); + return { objects, degraded, errors }; + }, + async describeObject(name: string): Promise { const def = (await metadataService.getObject(name)) as ObjectDef | undefined | null; if (!def) return null; diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 80c798980d..b6e1a461cf 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -483,15 +483,57 @@ export function buildMcpBridge(deps: DomainHandlerDeps, context: HttpProtocolCon const callData = actionExec.callData.bind(null, deps, context); const getMeta = () => deps.resolveService(context, 'metadata', envId); + const listObjectSummaries = async (): Promise => { + const meta: any = await getMeta(); + const objs: any[] = (await meta?.listObjects?.()) ?? []; + return objs.map((o) => ({ + name: o.name, + label: o.label ?? o.name, + fieldCount: o.fields ? Object.keys(o.fields).length : undefined, + })); + }; + return { - listObjects: async () => { + listObjects: listObjectSummaries, + /** + * [#6504] The HTTP transport's half of the `list_objects` completeness + * fix — the stdio bridge (`packages/mcp/src/stdio-data-bridge.ts`) + * carries the identical member, because the tool that renders + * `totalCount` is shared and a claim must not depend on which transport + * the client happened to connect over. + * + * `McpDataBridge` declares this member OPTIONAL, so implementing it here + * is what makes the tool's degraded branch reachable on this transport. + * The items come from the resolver directly above; only the verdict is + * asked of `listDiagnosed('object')`, the member declared to answer it + * — `listObjects` claims no equivalence to `list('object')`, and + * presuming one at a consumer is the private dialect Prime Directive #12 + * forbids. A metadata service predating `listDiagnosed` reports nothing + * degraded, which is exactly what it could express, and the tool then + * renders precisely what it rendered before. + * + * ⚠️ The verdict probe must not fail a read whose items already + * succeeded: a throw here would trade a working `list_objects` for + * observability. It is swallowed into "not degraded" — the same + * direction `warnIfSkillListIncomplete` takes above, and the only one + * that cannot manufacture a claim. + */ + listObjectsDiagnosed: async () => { + const objects = await listObjectSummaries(); const meta: any = await getMeta(); - const objs: any[] = (await meta?.listObjects?.()) ?? []; - return objs.map((o) => ({ - name: o.name, - label: o.label ?? o.name, - fieldCount: o.fields ? Object.keys(o.fields).length : undefined, - })); + if (!meta || typeof meta.listDiagnosed !== 'function') { + return { objects, degraded: false, errors: [] }; + } + try { + const diagnosed: any = await meta.listDiagnosed('object'); + return { + objects, + degraded: diagnosed?.degraded === true, + errors: Array.isArray(diagnosed?.errors) ? diagnosed.errors : [], + }; + } catch { + return { objects, degraded: false, errors: [] }; + } }, describeObject: async (name: string) => { const meta: any = await getMeta(); diff --git a/packages/runtime/src/external-validation-plugin.ts b/packages/runtime/src/external-validation-plugin.ts index 4057bb9707..4eb27aaa43 100644 --- a/packages/runtime/src/external-validation-plugin.ts +++ b/packages/runtime/src/external-validation-plugin.ts @@ -20,6 +20,105 @@ interface ExternalDatasourceServiceLike { interface MetadataServiceLike { get?: (type: string, name: string) => Promise; list?: (type: string) => Promise; + /** + * [#6504] The plural ADR-0110 D3 verdict — "could this listing be trusted as + * complete?". Optional exactly as on `IMetadataService`: a service predating + * it cannot report the distinction, and a service without it reports nothing + * degraded, which is precisely what it could express. + */ + listDiagnosed?: ( + type: string, + ) => Promise<{ items: unknown[]; degraded: boolean; errors: string[] }>; +} + +/** + * [#6504] Report the boot gate's all-clear — WITHOUT claiming universality over + * a set that may have been read short. + * + * ## The consumer's classification: mis-describing + * + * `validateAll()` sweeps `listObjects()` and filters it down to the federated + * objects. That listing goes silently short while a metadata loader is down + * (ADR-0110 D3), and this gate then makes two statements over the survivors: + * the sentence *all federated objects match their remote schema*, and the + * number `objects: N`. Both are positive claims about the ENVIRONMENT taken + * from a set nobody established was complete — the card's exact shape, one + * layer up from a `totalCount`. The federated objects held by an unreadable + * loader were never validated, and ADR-0015 §5.2's `onMismatch: 'fail'` gate + * therefore could not have fired for them: an outage silently narrows the gate + * and then announces a clean sweep. + * + * ## What changes, and what deliberately does not + * + * Only the CLAIM. The gate still validates and still refuses on every mismatch + * it found — a degraded read is a reason to withhold a completeness statement, + * never a reason to withhold the work, and never (see below) a reason to invent + * a failure. + * + * ⛔ It does **not** turn a degraded metadata read into a boot abort. That + * would convert a transient dependency outage into a refusal to start, which is + * a new functional failure mode bought with a diagnosis fix — the opposite of + * what #6504 is. The operator gets a `warn` naming the outage and the fact that + * the sweep was narrower than the environment, at the level AGENTS.md's + * degradation table asks for: the condition is visible and self-heals on the + * next boot after the loader does. + * + * The verdict is asked of the metadata service DIRECTLY rather than threaded + * through `SchemaValidationReport`, for two reasons. The plain one: that report + * is declared in `packages/spec`, whose surface this card does not own. The + * better one: the question is about the object listing, and the metadata + * service is where the answer lives — routing it through a second contract + * would add a member every implementer must remember to fill in, to relay a + * fact the authority can already be asked for. + * + * A verdict probe that THROWS must not turn a successful validation sweep into + * a failure, so it is reported as "could not be determined" — never flattened + * into a completeness claim this code did not earn. + */ +async function announceAllClear( + ctx: PluginContext, + metadata: MetadataServiceLike | undefined, + validated: number, +): Promise { + if (typeof metadata?.listDiagnosed !== 'function') { + ctx.logger?.info?.('[external-validation] all federated objects match their remote schema', { + objects: validated, + }); + return; + } + + let degraded = false; + let errors: string[] = []; + try { + const diagnosed = await metadata.listDiagnosed('object'); + degraded = diagnosed?.degraded === true; + errors = Array.isArray(diagnosed?.errors) ? diagnosed.errors : []; + } catch (err) { + ctx.logger?.warn?.( + '[external-validation] validated every federated object it could see, but whether that set was ' + + 'COMPLETE could not be determined — the metadata service\'s diagnosed read failed. Treat this ' + + 'boot as unverified for federated objects held by loaders that may have been unreachable.', + { validated, err }, + ); + return; + } + + if (!degraded) { + ctx.logger?.info?.('[external-validation] all federated objects match their remote schema', { + objects: validated, + }); + return; + } + + ctx.logger?.warn?.( + '[external-validation] schema validation swept an INCOMPLETE object set — the metadata service ' + + 'could not be fully read, so federated objects held by the unreachable loader(s) were never ' + + 'validated and the onMismatch gate could not have fired for them. Every object that WAS read ' + + `matches its remote schema (${validated} validated); this is not an all-clear for the ` + + 'environment. Fix: check the loaders behind the metadata service (datasource connection, ' + + 'credentials, table), then restart to re-run the gate.', + { validated, errors }, + ); } interface DatasourceDef { @@ -104,9 +203,10 @@ export class ExternalValidationPlugin implements Plugin { const failures = report.results.filter((r) => !r.ok); if (failures.length === 0) { - ctx.logger?.info?.('[external-validation] all federated objects match their remote schema', { - objects: report.results.length, - }); + // [#6504] The all-clear is a UNIVERSAL claim, and this gate has no way to + // make one when the object set it swept was itself known-partial. See + // `announceAllClear`. + await announceAllClear(ctx, metadata, report.results.length); return; } diff --git a/packages/runtime/src/list-diagnosed-consumer-sweep.test.ts b/packages/runtime/src/list-diagnosed-consumer-sweep.test.ts new file mode 100644 index 0000000000..683f9e3f82 --- /dev/null +++ b/packages/runtime/src/list-diagnosed-consumer-sweep.test.ts @@ -0,0 +1,391 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6504 (consumer sweep) — `packages/runtime`'s two list-family consumers that + * make a claim, pinned against a REAL loader outage. + * + * --------------------------------------------------------------------------- + * Why these two, and not every `list()` caller in the package + * --------------------------------------------------------------------------- + * The card's discipline (PR #6051, followed by PR #7721) is that each consumer + * of a possibly-short listing is qualified INDIVIDUALLY — gating, non-gating, + * or mis-describing — and that a blanket switch would be worse than leaving + * them alone. `packages/runtime` reads the metadata service's list family at + * six places. Four publish no claim about the environment and are correct + * unchanged; the two pinned here are the ones that state something a short read + * makes false: + * + * 1. **the MCP `list_objects` bridge** (`domains/mcp.ts`) — its tool renders + * `{ objects, totalCount }`, and `totalCount` is a positive, numeric claim + * about what this environment declares. Mis-describing, and the same shape + * PR #7721 closed on the `objectstack://objects` RESOURCE — the identical + * question over the other transport. + * 2. **the ADR-0015 §5.2 boot gate** (`external-validation-plugin.ts`) — it + * announces *all federated objects match their remote schema*, with a + * count, over whatever `validateAll()` could enumerate. Mis-describing AND + * gating: the objects held by an unreadable loader were never validated, so + * `onMismatch: 'fail'` could not have fired for them, and the boot is + * announced clean anyway. + * + * The rest of the package's inventory, with the reason each is left alone, is + * in the PR body. + * + * --------------------------------------------------------------------------- + * Why the failure is REAL and not stubbed + * --------------------------------------------------------------------------- + * `packages/runtime` depends on `@objectstack/metadata`, so — unlike the + * `packages/mcp` and `packages/services/service-datasource` halves of this + * sweep, which say so in their own headers — no double is needed anywhere in + * this file. Every degraded case below is produced by a real `MetadataManager` + * whose `DatabaseLoader` sits over a driver whose `find()` throws `ECONNRESET`, + * exactly as PR #7721's producer pin does. The `catch` in `readListUncached()` + * is therefore the thing under test and `degraded` is COMPUTED, not injected. A + * test that handed the consumer a pre-made verdict would prove only that a + * boolean can be passed along, which was never in doubt. + * + * --------------------------------------------------------------------------- + * What is asserted, and why it is the COUNT + * --------------------------------------------------------------------------- + * Both directions, on the number itself. The load-bearing pair is *"the outage + * and the small environment are the same listing"* (equal items AND equal + * length through the undiagnosed read — the defect, deliberately still true, + * because the plain read is unchanged) against *"the diagnosed read separates + * them"*. A pin 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 gives the flag meaning, and pinning the healed count (1 vs + * 3) is what shows the size of the lie a `totalCount` consumer told. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Ordinary red, in two independent ablations — one per consumer, since a single + * revert of both would not say which pin measures which decision. + * + * (a) delete `listObjectsDiagnosed` from `buildMcpBridge` — predicted **4 red + * / 5 green** of the 9. The four MCP cases that read the bridge's + * diagnosed member go red (vitest transpiles rather than type-checks, so + * the missing member surfaces as a runtime `TypeError`); green are the one + * "the plain read is unchanged" invariant — which must stay green in both + * directions, since a regression there would be a different bug — plus all + * four boot-gate cases, which never touch this consumer. + * (b) restore `announceAllClear`'s body to the unconditional + * `logger.info('… all federated objects match …')` — predicted **2 red / 7 + * green**. Red are exactly the two gate cases that discriminate on the + * withheld claim (the degraded one and the probe-throws one). The other + * two gate cases are green ON PURPOSE and are the reason the ablation is + * worth running: they assert that the claim is WITHHELD rather than + * removed, so a build that never learned to withhold it satisfies them + * too. The five MCP cases are untouched. + * + * Measured results are recorded in the PR body as they came out. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { MetadataManager, DatabaseLoader, MemoryLoader } from '@objectstack/metadata'; +import { HttpDispatcher } from './http-dispatcher.js'; +import { ExternalValidationPlugin } from './external-validation-plugin.js'; + +const connectionReset = (): Error => + Object.assign(new Error('read ECONNRESET'), { code: 'ECONNRESET' }); + +/** An `object` row as both the registry and the loader hand them back. */ +interface NamedObject { + name: string; + datasource?: string; +} + +const names = (items: unknown[]): string[] => + (items as NamedObject[]).map((i) => i.name).sort(); + +/** + * A `sys_metadata` store that fails every read until `heal()`, then serves two + * federated `object` rows. + * + * Two rows rather than 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 of 2 is + * exactly what a `totalCount` consumer understated. Both stored rows carry + * `datasource: 'warehouse'`, which is what makes them FEDERATED — the boot gate + * below filters on precisely that field, so an outage removes objects the gate + * was supposed to validate rather than objects it would have skipped anyway. + */ +function healableStore(): { driver: IDataDriver; heal: () => void } { + let broken = true; + const row = (name: string): Record => ({ + id: name, + name, + type: 'object', + metadata: JSON.stringify({ name, datasource: 'warehouse' }), + }); + const find = vi.fn(async (): Promise[]> => { + if (broken) throw connectionReset(); + return [row('wh_order'), row('wh_line')]; + }); + 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; } }; +} + +/** A real manager whose one `DatabaseLoader` is down, plus one registry object. */ +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, + // so the manager's list cache is the only memo in play. + manager.registerLoader(new DatabaseLoader({ driver: store.driver, cache: { enabled: false } })); + manager.registerInMemory('object', 'local_task', { name: 'local_task' }); + return { manager, heal: store.heal }; +} + +/** + * A manager that is genuinely small: one declaration, every loader answering. + * Its `listObjects()` is the answer the broken one above IMITATES. + */ +function managerOverHealthyStore(): MetadataManager { + const manager = new MetadataManager({ formats: ['json'], loaders: [new MemoryLoader()] }); + manager.registerInMemory('object', 'local_task', { name: 'local_task' }); + return manager; +} + +// ── The MCP bridge half ────────────────────────────────────────────────────── + +/** Build the kernel the dispatcher reads, with a REAL metadata service in the slot. */ +function makeKernel(metadata: unknown) { + const mcpService: any = { + lastOpts: undefined, + handleHttpRequest: async (_req: Request, o: any) => { + mcpService.lastOpts = o; + return new Response('{}', { status: 200, headers: { 'content-type': 'application/json' } }); + }, + }; + const services: Record = { mcp: mcpService, metadata }; + return { + getService: (n: string) => services[n], + getServiceAsync: async (n: string) => services[n], + } as any; +} + +function makeContext() { + return { + request: new Request('http://localhost/api/v1/mcp', { + method: 'POST', + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, + body: '{}', + }), + response: {}, + environmentId: undefined, + executionContext: { userId: 'u1', isSystem: false, positions: [], permissions: [] }, + }; +} + +/** Drive the real HTTP entry point and hand back the bridge the runtime built. */ +async function bridgeFor(metadata: unknown): Promise { + const kernel = makeKernel(metadata); + const d = new HttpDispatcher(kernel, undefined, { enforceProjectMembership: false }); + const res = await d.handleMcp({ jsonrpc: '2.0', id: 1, method: 'tools/list' }, makeContext() as any); + expect(res.response?.status, 'precondition: the MCP route must have been served').toBe(200); + return (kernel.getService('mcp') as any).lastOpts.bridge; +} + +describe('#6504 — the MCP object bridge: an outage must not arrive as a small environment', () => { + const prev = process.env.OS_MCP_SERVER_ENABLED; + beforeEach(() => { + process.env.OS_MCP_SERVER_ENABLED = 'true'; + // The manager's own outage line would otherwise print once per read. The + // verdict under test is the RETURN VALUE, not the log, so this silences + // noise without hiding anything the assertions depend on. + vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'info').mockImplementation(() => {}); + }); + afterEach(() => { + if (prev === undefined) delete process.env.OS_MCP_SERVER_ENABLED; + else process.env.OS_MCP_SERVER_ENABLED = prev; + vi.restoreAllMocks(); + }); + + it('the plain listObjects() cannot tell them apart — same objects, same COUNT', async () => { + const { manager: broken } = managerOverBrokenStore(); + const outage = await (await bridgeFor(broken)).listObjects(); + const small = await (await bridgeFor(managerOverHealthyStore())).listObjects(); + + // The defect stated as an assertion — and deliberately STILL TRUE after the + // fix, because `listObjects` is unchanged in every direction. A case that + // went red here would be reporting a regression, not this fix. + expect(names(outage)).toEqual(names(small)); + expect(outage).toHaveLength(small.length); + expect(names(outage)).toEqual(['local_task']); + }); + + it('listObjectsDiagnosed() separates them, and the COUNT is what the claim rests on', async () => { + const { manager: broken } = managerOverBrokenStore(); + const outage = await (await bridgeFor(broken)).listObjectsDiagnosed(); + const small = await (await bridgeFor(managerOverHealthyStore())).listObjectsDiagnosed(); + + expect(outage.degraded).toBe(true); + expect(small.degraded).toBe(false); + + // Same objects, same length — the two reads are indistinguishable on the + // data, which is exactly why the verdict has to travel beside it. + expect(names(outage.objects)).toEqual(names(small.objects)); + expect(outage.objects).toHaveLength(1); + expect(small.objects).toHaveLength(1); + + // The loader that was lost is named, so an operator can act on it. + expect(outage.errors).toHaveLength(1); + expect(outage.errors[0]).toMatch(/ECONNRESET/); + expect(small.errors).toEqual([]); + }); + + it('the outage is a MEASURABLE subtraction: the true count is 3, the claim would have said 1', async () => { + const { manager: broken } = managerOverBrokenStore(); + const { manager: working, heal } = managerOverBrokenStore(); + heal(); + + const degraded = await (await bridgeFor(broken)).listObjectsDiagnosed(); + const complete = await (await bridgeFor(working)).listObjectsDiagnosed(); + + expect(degraded.objects).toHaveLength(1); + expect(complete.objects).toHaveLength(3); + expect(complete.degraded).toBe(false); + // The gap a `totalCount` consumer would have published as fact. + expect(degraded.objects.length).toBeLessThan(complete.objects.length); + }); + + it('a metadata service PREDATING listDiagnosed reports nothing degraded — the member is optional', async () => { + // Not a double of the verdict: a real object listing, from a service that + // simply cannot express the distinction. Its behaviour must be exactly what + // it was before #6504, which is what keeps the optional member optional. + const legacy = { + listObjects: async () => [{ name: 'local_task' }], + getObject: async () => null, + list: async () => [], + }; + const read = await (await bridgeFor(legacy)).listObjectsDiagnosed(); + expect(read.degraded).toBe(false); + expect(read.errors).toEqual([]); + expect(names(read.objects)).toEqual(['local_task']); + }); + + it('a verdict probe that THROWS does not fail a read whose objects already succeeded', async () => { + // Trading a working `list_objects` for observability would be a new failure + // mode bought with a diagnosis fix. It must degrade to "nothing claimed". + const flaky = { + listObjects: async () => [{ name: 'local_task' }], + getObject: async () => null, + list: async () => [], + listDiagnosed: async () => { throw new Error('probe exploded'); }, + }; + const read = await (await bridgeFor(flaky)).listObjectsDiagnosed(); + expect(names(read.objects)).toEqual(['local_task']); + expect(read.degraded).toBe(false); + }); +}); + +// ── The ADR-0015 boot-gate half ────────────────────────────────────────────── + +/** + * The gate's context, with a REAL metadata service in the `metadata` slot and a + * federation service that validates cleanly. The validation result is fixed at + * "everything I was given matches" on purpose: the question under test is not + * whether the gate detects drift, it is whether the gate is entitled to call + * that result an ALL-CLEAR for the environment. + */ +function gateCtx(metadata: unknown, validated: string[]) { + const infos: any[] = []; + const warnings: any[] = []; + const services: Record = { + 'external-datasource': { + validateAll: async () => ({ + ok: true, + results: validated.map((object) => ({ ok: true, datasource: 'warehouse', object, diffs: [] })), + }), + }, + metadata, + }; + const ctx = { + getService: (name: string): T => { + if (name in services) return services[name] as T; + throw new Error(`service '${name}' not registered`); + }, + registerService: vi.fn(), + hook: vi.fn(), + trigger: vi.fn(), + logger: { + debug: vi.fn(), + info: (...a: any[]) => infos.push(a), + warn: (...a: any[]) => warnings.push(a), + }, + } as any; + return { ctx, infos, warnings }; +} + +const said = (lines: any[], fragment: string): boolean => + lines.some((l) => String(l[0]).includes(fragment)); + +describe('#6504 — the ADR-0015 boot gate must not announce an all-clear over an incomplete sweep', () => { + beforeEach(() => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'info').mockImplementation(() => {}); + }); + afterEach(() => vi.restoreAllMocks()); + + it('withholds the "all federated objects match" claim while a loader is down', async () => { + const { manager } = managerOverBrokenStore(); + // The sweep saw only the one object the registry holds; `wh_order` and + // `wh_line` — both federated — were never validated because they were never + // listed. The gate must not speak for them. + const { ctx, infos, warnings } = gateCtx(manager, ['local_task']); + + await new ExternalValidationPlugin().runValidation(ctx); + + expect(said(infos, 'all federated objects match'), 'the all-clear must NOT be claimed').toBe(false); + expect(said(warnings, 'INCOMPLETE object set')).toBe(true); + // The number it does state is named as what was VALIDATED, never as a total. + expect(said(warnings, '1 validated')).toBe(true); + }); + + it('makes the claim normally once every loader answers — this is a claim WITHHELD, not removed', async () => { + const { manager, heal } = managerOverBrokenStore(); + heal(); + const { ctx, infos, warnings } = gateCtx(manager, ['local_task', 'wh_order', 'wh_line']); + + await new ExternalValidationPlugin().runValidation(ctx); + + expect(said(infos, 'all federated objects match')).toBe(true); + expect(warnings, 'a complete sweep warns about nothing').toEqual([]); + }); + + it('still claims the all-clear on a service predating listDiagnosed — unchanged behaviour', async () => { + const legacy = { get: async () => undefined, list: async () => [] }; + const { ctx, infos } = gateCtx(legacy, ['wh_order']); + + await new ExternalValidationPlugin().runValidation(ctx); + + expect(said(infos, 'all federated objects match')).toBe(true); + }); + + it('says the completeness could not be DETERMINED when the probe itself throws', async () => { + // Neither an all-clear nor a degradation report: the honest third answer. + const flaky = { + get: async () => undefined, + list: async () => [], + listDiagnosed: async () => { throw new Error('probe exploded'); }, + }; + const { ctx, infos, warnings } = gateCtx(flaky, ['wh_order']); + + await new ExternalValidationPlugin().runValidation(ctx); + + expect(said(infos, 'all federated objects match')).toBe(false); + expect(said(warnings, 'could not be determined')).toBe(true); + }); +}); diff --git a/packages/services/service-datasource/src/__tests__/datasource-removal-bound-count-outage.test.ts b/packages/services/service-datasource/src/__tests__/datasource-removal-bound-count-outage.test.ts new file mode 100644 index 0000000000..b6be2dd4b1 --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/datasource-removal-bound-count-outage.test.ts @@ -0,0 +1,299 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6504 (consumer sweep) — `removeDatasource`'s bound-object guard is a COUNT + * spent as a safety verdict, so an under-count opens it. + * + * --------------------------------------------------------------------------- + * Why this consumer is the sharpest one in the sweep + * --------------------------------------------------------------------------- + * The card's thesis is that during a loader outage a plural read serves "fewer + * items, with a count", and that machine consumers trust counts. Every other + * consumer in this sweep publishes that count to a reader who may believe it. + * This one SPENDS it, on the only guard standing in front of an irreversible + * operation: + * + * const bound = await countBoundObjects(name); // ← from listObjects() + * if (bound > 0) throw … // ← the whole guard + * await deleteDatasourceRecord(name); // ← irreversible + * await removeSecret(existing.external.credentialsRef); + * + * `countBoundObjects` derives its number from the metadata service's object + * listing, which goes silently short while a loader is down (ADR-0110 D3). Its + * worst value is the benign-looking one: `0` is indistinguishable from "nothing + * is bound", so the guard does not merely mis-state — it OPENS, the datasource + * is deleted, and its credential is unbound behind it, while the objects that + * were still bound to it were simply unreadable at that moment. + * + * --------------------------------------------------------------------------- + * DOUBLES HERE, and the split is deliberate — the real-loader pin is elsewhere + * --------------------------------------------------------------------------- + * `packages/services/service-datasource` does not depend on + * `@objectstack/metadata`, and adding that dependency so a unit test could + * construct a `MetadataManager` would be a far larger change than the fix. So + * the verdict reaching `DatasourceAdminService` is injected here, exactly as + * PR #7721 did for `packages/mcp` and for the same reason, and it is stated + * plainly rather than papered over. + * + * What that leaves un-pinned in THIS file is only the production of the + * verdict, and that is pinned twice elsewhere, against a real `DatabaseLoader` + * over a driver throwing `ECONNRESET`: + * `packages/metadata/src/metadata-manager-list-diagnosed.test.ts` (the + * producer) and `packages/runtime/src/list-diagnosed-consumer-sweep.test.ts` + * (this sweep's real-failure consumer pin). What IS pinned here is the decision + * that only exists in this file: what a datasource removal does when the count + * behind its guard cannot be trusted. + * + * The wiring that composes the two — `countBoundObjectsDiagnosed` in + * `datasource-admin-plugin.ts`, which takes the items from `listObjects()` and + * the verdict from `listDiagnosed('object')` — is exercised by its own case at + * the bottom of this file, over a metadata-service double, so the composition + * is not merely asserted in a comment. + * + * --------------------------------------------------------------------------- + * Both directions, on the COUNT + * --------------------------------------------------------------------------- + * The load-bearing pair is `count: 0, degraded: true` (the outage that reads as + * "nothing is bound") against `count: 0, degraded: false` (a datasource that + * genuinely has nothing bound). The two are BYTE-EQUAL on the number, and the + * removal must go opposite ways on them. A test that only asserted "a degraded + * flag exists" would pass on a build that ignored it at the decision. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Ordinary red. Reversion is defined as restoring + * `const bound = await this.config.countBoundObjects(name)` and the plain + * `bound > 0` guard — i.e. the pre-#6504 consumer, with + * `countBoundObjectsDiagnosed` left declared but unread. + * + * Predicted, written down before running: **3 red / 5 green** of the 8. Red are + * the three cases that assert the refusal — the throw, its + * 503/`SERVICE_UNAVAILABLE` envelope, and the record + secret + pool surviving + * it. Green are the three that assert UNCHANGED behaviour (the genuinely-empty + * removal still succeeding, the original bound-objects refusal keeping its own + * message, and a host without the diagnosed member behaving as before), plus + * the two wiring cases in the second describe — the plugin still WIRES the + * diagnosed member under this ablation, it is the service that stops reading + * it, which is exactly the failure shape a "declared but unconsumed" surface + * has. + * + * **First measurement: 4 red / 4 green — the prediction was wrong**, and + * usefully so. The extra red was *"a complete read that finds bindings still + * refuses"*: the harness supplied that case's count ONLY through the diagnosed + * member, so the ablated build read the plain count as `0` and removed the + * datasource. That is a fixture where the two counts disagree, which measures + * the wiring rather than the decision — on the shipped wiring they are the same + * filter over the same listing. The harness now derives the plain count from + * the diagnosed one so the only difference between the two directions is + * `degraded`. **Re-measured: 3 red / 5 green**, as predicted. Both numbers are + * recorded because the first one is the one that found the incoherent fixture. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + DatasourceAdminService, + type DatasourceAdminServiceConfig, + type StoredDatasource, +} from '../datasource-admin-service.js'; + +const RUNTIME_ROW: StoredDatasource = { + name: 'warehouse', + driver: 'postgres', + origin: 'runtime', + external: { credentialsRef: 'sys_secret://datasource/warehouse#1' }, +} as StoredDatasource; + +interface HarnessOpts { + /** The diagnosed count the host reports. Omit to model a host predating it. */ + diagnosed?: { count: number; degraded: boolean; errors: string[] }; + /** The plain count, always wired — it is the pre-#6504 path. */ + plainCount?: number; +} + +function harness(opts: HarnessOpts = {}) { + const records: StoredDatasource[] = [{ ...RUNTIME_ROW }]; + const removedSecrets: string[] = []; + const unregistered: string[] = []; + + const config: DatasourceAdminServiceConfig = { + probe: async () => ({ ok: true }), + listDatasourceRecords: async () => records, + getDatasourceRecord: async (n) => records.find((r) => r.name === n), + putDatasourceRecord: async () => {}, + deleteDatasourceRecord: async (n) => { + const i = records.findIndex((r) => r.name === n); + if (i >= 0) records.splice(i, 1); + }, + writeSecret: async () => 'sys_secret://unused', + removeSecret: async (ref) => { removedSecrets.push(ref); }, + // The plain count AGREES with the diagnosed one by construction. On the + // shipped wiring they are the same filter over the same listing, and a + // fixture where they disagree measures the wiring rather than the decision + // — see this file's reverse-verification note, where an earlier version + // that left this at 0 produced an extra red for exactly that reason. + countBoundObjects: async () => opts.plainCount ?? opts.diagnosed?.count ?? 0, + ...(opts.diagnosed ? { countBoundObjectsDiagnosed: async () => opts.diagnosed! } : {}), + unregisterPool: (n) => { unregistered.push(n); }, + } as DatasourceAdminServiceConfig; + + return { + service: new DatasourceAdminService(config), + exists: () => records.some((r) => r.name === 'warehouse'), + removedSecrets, + unregistered, + }; +} + +const LOADER_FAILURE = 'database: read ECONNRESET'; + +describe('#6504 — a datasource removal refuses when the bound-object count is known-partial', () => { + it('REFUSES on a degraded read whose count is 0 — the value that reads as "nothing is bound"', async () => { + const h = harness({ diagnosed: { count: 0, degraded: true, errors: [LOADER_FAILURE] } }); + + await expect(h.service.removeDatasource('warehouse')).rejects.toThrow( + /could not be fully read/, + ); + }); + + it('carries the ADR-0112 envelope: SERVICE_UNAVAILABLE / 503, not a 400-class refusal', async () => { + // The distinction is the whole point of the envelope: nothing about the + // REQUEST is wrong, the condition is a dependency outage that may clear, and + // the caller SHOULD retry. A bare Error would land as this service's generic + // 400 `DATASOURCE_ADMIN_ERROR` and tell the operator the opposite. + const h = harness({ diagnosed: { count: 0, degraded: true, errors: [LOADER_FAILURE] } }); + + const err = await h.service.removeDatasource('warehouse').catch((e) => e); + + expect(err).toBeInstanceOf(Error); + expect((err as { code?: string }).code).toBe('SERVICE_UNAVAILABLE'); + expect((err as { status?: number }).status).toBe(503); + // The first sentence is contract here — it is what the operator reads on the + // API response — so it is asserted on top of the code/status, not instead. + expect((err as Error).message).toMatch(/Cannot remove datasource 'warehouse'/); + expect((err as Error).message).toMatch(/the true number can only be higher/); + // The loader detail rides on `cause`, never in the served message: it names + // internal datasources and tables. + expect(String((err as { cause?: unknown }).cause)).toContain('ECONNRESET'); + expect((err as Error).message).not.toContain('ECONNRESET'); + }); + + it('leaves the record AND its secret intact — the refusal is the whole point', async () => { + const h = harness({ diagnosed: { count: 0, degraded: true, errors: [LOADER_FAILURE] } }); + + await h.service.removeDatasource('warehouse').catch(() => undefined); + + expect(h.exists(), 'the datasource must survive a refusal').toBe(true); + expect(h.removedSecrets, 'its credential must not be unbound').toEqual([]); + expect(h.unregistered, 'its pool must not be torn down').toEqual([]); + }); + + it('a COMPLETE read of the same count 0 removes it — byte-equal number, opposite outcome', async () => { + // The pair that gives the verdict meaning. `count: 0` in both cases; only + // `degraded` differs, and it must be what decides. + const h = harness({ diagnosed: { count: 0, degraded: false, errors: [] } }); + + await expect(h.service.removeDatasource('warehouse')).resolves.toBeUndefined(); + + expect(h.exists()).toBe(false); + expect(h.removedSecrets).toEqual(['sys_secret://datasource/warehouse#1']); + }); + + it('a complete read that finds bindings still refuses with the ORIGINAL message', async () => { + // The pre-existing guard is untouched: this change adds a second reason to + // refuse, it does not re-word or weaken the first. + const h = harness({ diagnosed: { count: 2, degraded: false, errors: [] } }); + + await expect(h.service.removeDatasource('warehouse')).rejects.toThrow( + /2 object\(s\) are still bound to it/, + ); + expect(h.exists()).toBe(true); + }); + + it('a host predating `countBoundObjectsDiagnosed` behaves exactly as before', async () => { + // The optionality `IMetadataService.listDiagnosed` itself carries, one layer + // out: a host that cannot report the distinction reports nothing degraded, + // and its removals are unchanged in both directions. + const empty = harness({ plainCount: 0 }); + await expect(empty.service.removeDatasource('warehouse')).resolves.toBeUndefined(); + expect(empty.exists()).toBe(false); + + const bound = harness({ plainCount: 3 }); + await expect(bound.service.removeDatasource('warehouse')).rejects.toThrow( + /3 object\(s\) are still bound to it/, + ); + expect(bound.exists()).toBe(true); + }); +}); + +describe('#6504 — the plugin wiring composes the count and the verdict correctly', () => { + /** + * The composition PR #7721 established: the ITEMS come from the resolver the + * call site already used (`listObjects()`), and only the VERDICT is asked of + * `listDiagnosed('object')`. Re-resolving the objects through the diagnosed + * read would presume `listObjects()` and `list('object')` are the same read — + * an equivalence `IMetadataService` does not declare. + * + * Built by calling the plugin's own `init` and reading the config it wired, so + * this pins the shipped wiring rather than a restatement of it. + */ + async function wiredConfig(metadata: unknown): Promise { + const { DatasourceAdminServicePlugin } = await import('../datasource-admin-plugin.js'); + const plugin = new DatasourceAdminServicePlugin(); + const services: Record = { metadata }; + const ctx = { + getService: (n: string) => { + if (n in services) return services[n]; + throw new Error(`service '${n}' not registered`); + }, + registerService: vi.fn(), + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as any; + await plugin.init(ctx); + return (plugin as unknown as { config: DatasourceAdminServiceConfig }).config; + } + + it('counts from `listObjects()` and takes the verdict from `listDiagnosed("object")`', async () => { + const diagnosedCalls: string[] = []; + const metadata = { + get: async () => undefined, + list: async () => [], + register: async () => {}, + unregister: async () => {}, + // Two bound objects are readable; a third is behind the dead loader. + listObjects: async () => [ + { name: 'wh_order', datasource: 'warehouse' }, + { name: 'wh_line', datasource: 'warehouse' }, + { name: 'local_task' }, + ], + listDiagnosed: async (type: string) => { + diagnosedCalls.push(type); + return { items: [], degraded: true, errors: [LOADER_FAILURE] }; + }, + }; + + const config = await wiredConfig(metadata); + const read = await config.countBoundObjectsDiagnosed!('warehouse'); + + expect(read.count, 'the count comes from listObjects(), filtered by datasource').toBe(2); + expect(read.degraded).toBe(true); + expect(read.errors).toEqual([LOADER_FAILURE]); + expect(diagnosedCalls, 'the verdict is asked for `object`, the type being counted').toEqual([ + 'object', + ]); + }); + + it('reports nothing degraded against a metadata service predating `listDiagnosed`', async () => { + const config = await wiredConfig({ + get: async () => undefined, + list: async () => [], + register: async () => {}, + unregister: async () => {}, + listObjects: async () => [{ name: 'wh_order', datasource: 'warehouse' }], + }); + + const read = await config.countBoundObjectsDiagnosed!('warehouse'); + + expect(read).toEqual({ count: 1, degraded: false, errors: [] }); + }); +}); diff --git a/packages/services/service-datasource/src/admin-routes.ts b/packages/services/service-datasource/src/admin-routes.ts index 0e67e9dafa..d0525574cd 100644 --- a/packages/services/service-datasource/src/admin-routes.ts +++ b/packages/services/service-datasource/src/admin-routes.ts @@ -155,9 +155,27 @@ export function registerDatasourceAdminRoutes( * route dispatches to (`SERVICE_ERROR_CODE`) and the service's own message. * `service` is the same name the route passed to `resolve` — restating it is * what keeps the attribution honest per route (#4249). + * + * [#6504] One exception, and it is a relay rather than a new decision: a + * service that threw an error already carrying the `503`/`SERVICE_UNAVAILABLE` + * envelope has classified its own refusal as a DEPENDENCY OUTAGE, and 400 + * would tell the caller its request was malformed — the opposite of the + * truth, and the opposite of "retry this". The only thrower today is + * `removeDatasource` refusing to delete on a bound-object count it could not + * take completely. Read off the error rather than special-cased per route, so + * the next refusal of this class needs no second edit here; both fields are + * required so an unrelated error carrying a stray `status` cannot re-route + * itself. `sendError`'s parameter type is the closed `ErrorCode` union, so + * the code below is checked at compile time rather than trusted from the + * throw site. */ - const badRequest = (res: any, service: ServiceName, err: unknown) => - sendError(res, 400, SERVICE_ERROR_CODE[service], err instanceof Error ? err.message : String(err)); + const badRequest = (res: any, service: ServiceName, err: unknown) => { + const envelope = err as { code?: unknown; status?: unknown } | null | undefined; + if (envelope?.status === 503 && envelope?.code === 'SERVICE_UNAVAILABLE') { + return sendError(res, 503, 'SERVICE_UNAVAILABLE', (err as Error).message); + } + return sendError(res, 400, SERVICE_ERROR_CODE[service], err instanceof Error ? err.message : String(err)); + }; /** Split an inline `{ secret, ...draft }` body into (draft, secret). */ const splitSecret = (body: any): { draft: any; secret: any } => { diff --git a/packages/services/service-datasource/src/datasource-admin-plugin.ts b/packages/services/service-datasource/src/datasource-admin-plugin.ts index dad8ac1b7a..c6e7d76f71 100644 --- a/packages/services/service-datasource/src/datasource-admin-plugin.ts +++ b/packages/services/service-datasource/src/datasource-admin-plugin.ts @@ -31,6 +31,13 @@ interface MetadataServiceLike { register: (type: string, name: string, data: unknown) => Promise; unregister: (type: string, name: string) => Promise; listObjects?: () => Promise; + /** + * [#6504] The plural ADR-0110 D3 verdict. Optional on this structural type + * for the reason it is optional on `IMetadataService` itself: a service that + * predates it cannot report the distinction, so its absence means the + * bound-object guard behaves exactly as it did before. + */ + listDiagnosed?: (type: string) => Promise<{ items: unknown[]; degraded: boolean; errors: string[] }>; } /** Engine surface used for hot pool (de)registration. */ @@ -340,6 +347,40 @@ export class DatasourceAdminServicePlugin implements Plugin { return objects.filter((o) => o?.datasource === datasource).length; }, + // [#6504] The same count with the completeness verdict attached — see + // `DatasourceAdminService.removeDatasource` for why this one guard is + // worth the extra read while the neighbouring listing above is not. + // + // The composition is PR #7721's, deliberately: the items come from the + // resolver that already answers this question (`listObjects()`, falling + // back to `list('object')`), and only the verdict — "could that answer be + // trusted as complete?" — is asked of `listDiagnosed('object')`, the + // member declared to answer it. Re-resolving the objects THROUGH + // `listDiagnosed` instead would quietly assume `listObjects()` and + // `list('object')` are the same read; `IMetadataService` declares no such + // equivalence, and presuming one at a consumer is the private dialect + // Prime Directive #12 forbids. On the implementation that ships they are + // the same read and share one cache entry and one single-flight slot, so + // the probe costs nothing; on a host where they differ the verdict + // describes the loader set, which can only WITHHOLD a completeness claim, + // never manufacture one. + countBoundObjectsDiagnosed: async (datasource) => { + const metadata = metadataOf(); + const objects = ((await metadata?.listObjects?.()) ?? + (await metadata?.list('object')) ?? + []) as Array<{ datasource?: string }>; + const count = objects.filter((o) => o?.datasource === datasource).length; + if (typeof metadata?.listDiagnosed !== 'function') { + return { count, degraded: false, errors: [] }; + } + const diagnosed = await metadata.listDiagnosed('object'); + return { + count, + degraded: diagnosed?.degraded === true, + errors: Array.isArray(diagnosed?.errors) ? diagnosed.errors : [], + }; + }, + // Hot pool (de)registration converges on the shared // DatasourceConnectionService (ADR-0062 D1) — one connect path for code- // and runtime-origin datasources. `connect()` builds the driver via the diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index 3286dd021d..9e65dff836 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/packages/services/service-datasource/src/datasource-admin-service.ts @@ -24,7 +24,10 @@ * service, and is separate from the one above — rows written before #8078 * can and do hold inline cleartext, which is why it is stated on its own * rather than treated as a consequence. - * - Removal is refused while objects are still bound to the datasource. + * - Removal is refused while objects are still bound to the datasource — and + * (#6504) equally refused when the bound-object count could not be taken + * over a COMPLETE object set, since an under-count and "nothing is bound" + * are the same zero. */ import { validateDriverConfig } from '@objectstack/spec/data'; @@ -110,6 +113,28 @@ export interface DatasourceAdminServiceConfig { readSecret?: (credentialsRef: string) => Promise; /** Count objects bound to a datasource (removal blocked while > 0). */ countBoundObjects: (datasource: string) => Promise; + /** + * [#6504] The same count, plus whether the object set it was taken over could + * be read COMPLETELY. + * + * {@link countBoundObjects} is derived from the metadata service's object + * listing, and that listing is short — silently — while a loader is down + * (ADR-0110 D3). The number is therefore not merely an under-report: it is + * the input to a guard over a DESTRUCTIVE operation. `0` returned during an + * outage reads exactly like "nothing is bound", so + * {@link DatasourceAdminService.removeDatasource} deletes a datasource whose + * objects were simply unreadable, and unbinds its secret on the way out. + * This is the card's harm one step past description: a count nobody can + * check is being spent as a safety verdict. + * + * Optional for the reason `IMetadataService.listDiagnosed` itself is: a host + * whose metadata service predates the verdict cannot report the distinction, + * so its absence means "unchanged behaviour", never "complete". A host that + * supplies it gets a removal that refuses rather than guesses. + */ + countBoundObjectsDiagnosed?: ( + datasource: string, + ) => Promise<{ count: number; degraded: boolean; errors: string[] }>; /** Hot-(re)register a runtime datasource's connection pool after write. */ registerPool?: (record: StoredDatasource) => Promise | void; /** Tear down a runtime datasource's pool on remove. */ @@ -163,6 +188,29 @@ function withRemaining(remaining: string[]): { remaining?: string[] } { return remaining.length > 0 ? { remaining } : {}; } +/** + * [#6504] A refusal caused by a metadata read that could not be completed — + * carrying the ADR-0112 envelope so it does not land as a 400 alongside this + * service's genuine client errors. + * + * 503 / `SERVICE_UNAVAILABLE` for the reason `metadataStoreUnavailableError` + * (#5532) picks them in `metadata-protocol`: nothing about the REQUEST is + * wrong, the condition is a dependency outage that may clear, and a caller + * SHOULD retry. `SERVICE_UNAVAILABLE` is the standard catalog's own code for + * 503 (`HttpStatusErrorCodeMap[503]`), so this registers no new ledger + * vocabulary for a distinction the routes already know how to render. + * + * The loader messages ride on `cause` rather than in `message`: they name + * internal datasources and tables, and the message is served to an API caller. + */ +function metadataIncompleteError(message: string, errors: string[]): Error { + const err = new Error(message) as Error & { code?: string; status?: number; cause?: unknown }; + err.code = 'SERVICE_UNAVAILABLE'; + err.status = 503; + if (errors.length > 0) err.cause = new Error(errors.join('; ')); + return err; +} + export class DatasourceAdminService implements IDatasourceAdminService { constructor(private readonly config: DatasourceAdminServiceConfig) {} @@ -420,6 +468,37 @@ export class DatasourceAdminService implements IDatasourceAdminService { return this.toSummary(merged); } + /** + * [#6504] Remove a runtime datasource, refusing when the bound-object count + * behind the safety guard cannot be trusted. + * + * ## Why this consumer is GATING as well as mis-describing + * + * The card's sweep classifies each `list()`/`listObjects()` consumer by what + * it does with a possibly-short answer. Most publish a snapshot and are right + * unchanged. This one takes a **count** — the strongest positive claim a read + * can make — and spends it as the sole guard over an irreversible operation: + * the record is deleted and, on a ref-bearing row, the secret is unbound + * behind it. A loader outage makes `countBoundObjects` under-report, and its + * worst value is the benign-looking one: `0` is precisely "nothing is bound", + * so the guard does not merely mis-state, it OPENS. + * + * ## The direction of the refusal + * + * Withholding the destructive act is the plural analogue of the + * `objectstack://objects` fix, which withholds the `totalCount` claim while + * still serving every object it could read. Here there is no data to keep + * serving — the "answer" IS the deletion — so the honest response is to make + * none: refuse, name the outage, and let the operator retry once the loaders + * are back. It is fully reversible in a way the deletion is not, and a + * transient dependency outage is exactly the condition a 503 exists for. + * + * ⛔ Not a blanket "refuse whenever the metadata service is unhappy": a host + * without {@link DatasourceAdminServiceConfig.countBoundObjectsDiagnosed} + * behaves exactly as before, because a service that cannot report the + * distinction reports nothing degraded — the same optionality + * `IMetadataService.listDiagnosed` itself carries. + */ async removeDatasource(name: string): Promise { const existing = await this.config.getDatasourceRecord(name); if (!existing) throw new Error(`Datasource '${name}' not found.`); @@ -427,10 +506,23 @@ export class DatasourceAdminService implements IDatasourceAdminService { throw new Error(`Datasource '${name}' is code-defined and cannot be removed at runtime.`); } - const bound = await this.config.countBoundObjects(name); - if (bound > 0) { + const bound = this.config.countBoundObjectsDiagnosed + ? await this.config.countBoundObjectsDiagnosed(name) + : { count: await this.config.countBoundObjects(name), degraded: false, errors: [] }; + + if (bound.degraded) { + throw metadataIncompleteError( + `Cannot remove datasource '${name}': the metadata service could not be fully read, so the ` + + `number of objects bound to it is unknown (${bound.count} counted from the objects that ` + + 'WERE readable — the true number can only be higher). Removing it now could orphan ' + + 'objects that are still bound. Retry once the metadata service is reachable.', + bound.errors, + ); + } + + if (bound.count > 0) { throw new Error( - `Cannot remove datasource '${name}': ${bound} object(s) are still bound to it.`, + `Cannot remove datasource '${name}': ${bound.count} object(s) are still bound to it.`, ); }