Skip to content

Commit 8519095

Browse files
claude[bot]claude
andauthored
fix(rest): stop absorbing a failed registry read into a 200 on GET /api/v1/packages (#11130) (#11378)
The list door merges two sources -- the in-memory registry via `protocol.getMetaItems({ type: 'package' })` and the durable `sys_packages` rows via `PackageService.list()`. #11063 stopped the door swallowing a failure of the durable half; the registry half still carried its own bare `catch {}`, so the same ambiguity stayed open on the other source: a failed registry read was answered as a 200 whose `total` claimed to be a complete count, with the surviving entries marked `source: 'database'` -- provenance, not a warning. Establish-first (the card left this open): a PRESENT `getMetaItems` DOES have a reachable throw, and the producer ALREADY declares it. The live `protocol` service is `ObjectStackProtocolImplementation`, whose `getMetaItems` routes every non-benign `sys_metadata` read failure through `rethrowUnlessMetadataStoreUnprovisioned` -> `metadataStoreUnavailableError`: `SERVICE_UNAVAILABLE` / 503 with an ADR-0112 status+code on the error (#5532). So route (b)'s producer leg was already landed and only #11063's edit remained. An absent protocol service is untouched (still 200, durable half alone), first boot is untouched (`isMissingTableError` is not a throw), and no wire field is added -- shape (c) would be a contract decision this card does not carry. Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR Co-authored-by: os-zhuang <noreply@anthropic.com>
1 parent 9337cd8 commit 8519095

3 files changed

Lines changed: 356 additions & 23 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
"@objectstack/rest": patch
3+
---
4+
5+
**Fix:** `GET /api/v1/packages` no longer absorbs a failed **registry** read into a `200` — the other half of the two-source merge #11063 closed (#11130).
6+
7+
The list door merges two sources: the in-memory registry, read through `protocol.getMetaItems({ type: 'package' })`, and the durable `sys_packages` rows, read through `PackageService.list()`. #11063 stopped the door swallowing a failure of the durable half. The registry half still carried its own bare `catch {}` commented *"Protocol unavailable — continue with database only"*, so the exact ambiguity #11063 closed stayed open on the other source: when `getMetaItems` threw, the door answered `200` with `{ packages, total }` built from the database alone, and `total` was presented as a **complete count** either way. The surviving entries kept `source: 'database'`, which reads as provenance rather than as a warning that the registry half is absent — nothing on the wire separated *"these are all the packages"* from *"these are the packages I could still see"*. Same standing family ruling as its sibling — #10965 · #10677 / PR #10788 · #10789 / PR #10964 · #11063: **a read that could not happen must not be reported as a read that found nothing.**
8+
9+
**The producer already declared its refusal, so this is #11063's edit rather than a new posture.** The card left open whether a *present* `getMetaItems` even has a reachable throw. Measured on the live implementation — `ObjectStackProtocolImplementation` in `packages/metadata-protocol`, the object `MetadataProtocolPlugin` registers as the `protocol` service and the one `RestApiPlugin` hands to this registrar: it routes every non-benign `sys_metadata` overlay read failure through `rethrowUnlessMetadataStoreUnprovisioned``metadataStoreUnavailableError`, i.e. `SERVICE_UNAVAILABLE` / 503 with an ADR-0112 status+code **on the error** (#5532) — the same envelope #10965 gave `PackageService.list()`. Driving it with the door's own request answers `THREW status=503 code=SERVICE_UNAVAILABLE` on an `ECONNREFUSED` store. So route (b)'s first leg was already landed and only the second remained: stop swallowing.
10+
11+
**What changes on the wire.** A registry read that refuses now reaches the client as the producer's own `503` / `SERVICE_UNAVAILABLE` through the declared envelope, instead of a `200` whose `total` lied. A registry read that throws something *undeclared* is answered as the `500 INTERNAL_ERROR` a fault deserves, instead of being eaten. Both were previously indistinguishable from a healthy, complete listing.
12+
13+
**What deliberately does not change.** An **absent** protocol service is a different, already-handled case — the `if (options.protocol && typeof … === 'function')` guard is untouched, so a composition without the protocol service still answers `200` with the durable half alone. First boot is unaffected: `sys_metadata` not being provisioned yet is not a throw on that path (`isMissingTableError`), it is an empty overlay. The healthy merge — two sources, `source: 'registry' | 'database' | 'both'`, a truthful `total` — is byte-identical. The durable half is untouched (#11063 landed it), and so is the dispatcher twin in `packages/runtime/src/domains/packages.ts`, which serves a single in-memory read with nothing of this shape.
14+
15+
**No response field is added.** The card's third shape — keep the `200` and make the tolerance visible with a partial-result marker — is a response-shape change, i.e. a contract decision, and was not authorized here.
16+
17+
Pinned by `packages/rest/src/package-list-registry-read-refusal.test.ts`, which asserts the mechanism rather than "the door still answers": the status **and** the declared `code`, that no `total` is reported over a read that failed, that both halves of the one merge answer the same outage identically, and — as the overreach guards — that an absent protocol service and a healthy merge are unmoved. On the pre-fix tree 4 of its 6 cases fail, every one of them reading `200` where a refusal was owed.
Lines changed: 279 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,279 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #11130 — `GET /api/v1/packages` must not absorb a failed REGISTRY read either.
5+
*
6+
* ## What was wrong
7+
*
8+
* The list door merges TWO sources — the in-memory registry (via
9+
* `protocol.getMetaItems({ type: 'package' })`) and the durable `sys_packages`
10+
* rows (via `PackageService.list()`). #11063 stopped the door absorbing a
11+
* failure of the DURABLE half. The REGISTRY half still carried its own:
12+
*
13+
* } catch {
14+
* // Protocol unavailable — continue with database only
15+
* }
16+
*
17+
* so the exact ambiguity #11063 closed stayed open on the other half: when
18+
* `getMetaItems` threw, the door answered `200` with `{ packages, total }` built
19+
* from the database alone, and `total` was reported as a COMPLETE count either
20+
* way. The surviving entries kept `source: 'database'`, which reads as
21+
* PROVENANCE, not as a warning that the registry half is absent. Nothing on the
22+
* wire separated *"these are all the packages"* from *"these are the packages I
23+
* could still see"*.
24+
*
25+
* Standing family ruling — #10965 · #10677 / PR #10788 · #10789 / PR #10964 ·
26+
* #11063: **a read that could not happen must not be reported as a read that
27+
* found nothing.**
28+
*
29+
* ## Why the producer half of route (b) is already landed
30+
*
31+
* The card's route (b) is "teach the producer to declare a refusal, then stop
32+
* swallowing it". Measured before this change: the producer ALREADY declares
33+
* it. The live `protocol` service is
34+
* `ObjectStackProtocolImplementation` (`packages/metadata-protocol`), whose
35+
* `getMetaItems` routes every non-benign `sys_metadata` read failure through
36+
* `rethrowUnlessMetadataStoreUnprovisioned` → `metadataStoreUnavailableError`,
37+
* i.e. `SERVICE_UNAVAILABLE` / 503 with an ADR-0112 status+code on the error —
38+
* the same envelope #10965 gave `PackageService.list()`. That producer behaviour
39+
* is pinned on the REAL implementation in
40+
* `packages/metadata-protocol/src/protocol.metadata-store-outage.test.ts`
41+
* (#5532). So the only leg left for this door is #11063's: stop swallowing.
42+
*
43+
* ⚠️ Asserting "the listing returns 200" passes on the OLD code, on the fixed
44+
* code, and on a wrong fix. Every case below pins the MECHANISM instead: which
45+
* status and which declared `code` reach the client, that `total` is not
46+
* reported at all over a read that failed, and that the two HALVES of one merge
47+
* answer the same failure identically.
48+
*
49+
* The refusal is reproduced locally rather than imported, for the reason the
50+
* #11063 sibling states: this suite stays free of a cross-package VALUE import
51+
* (and of the build-state dependence it would carry) — and `packages/rest`
52+
* deliberately has no run-time dependency on `@objectstack/metadata-protocol`
53+
* at all. The shape it reproduces is `metadataStoreUnavailableError()` in
54+
* `packages/metadata-protocol/src/protocol.ts`.
55+
*
56+
* ⛔ No wire field is added by the fix and none is asserted here. Shape (c) —
57+
* keep the 200 and make the tolerance visible with a partial-result marker — is
58+
* a response-shape change, i.e. a contract decision this queue entry does not
59+
* carry.
60+
*/
61+
62+
import { describe, it, expect } from 'vitest';
63+
import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api';
64+
import type { RouteHandler } from '@objectstack/spec/contracts';
65+
import { registerPackageRoutes } from './package-routes.js';
66+
67+
const PKGS = '/api/v1/packages';
68+
69+
interface Captured {
70+
status: number;
71+
body: any;
72+
}
73+
74+
/** Only the methods this read door reaches. */
75+
type Svc = Partial<{
76+
list: () => Promise<any[]>;
77+
get: (id: string, version?: string) => Promise<any>;
78+
}>;
79+
80+
/**
81+
* The #5532 refusal `ObjectStackProtocolImplementation.getMetaItems` raises when
82+
* the `sys_metadata` overlay read fails for any reason other than "the table is
83+
* not provisioned yet", reproduced: an ADR-0112 envelope ON THE ERROR — a
84+
* declared `status` AND a declared `code` — which is what lets it leave through
85+
* the door's shared `resolveThrownHttpError` mapping as the PRODUCER's answer
86+
* rather than as a 500 catch-all.
87+
*/
88+
function metadataStoreUnavailable(): Error {
89+
return Object.assign(
90+
new Error(
91+
'The metadata store could not be read, so whether this item exists is unknown. '
92+
+ 'Retry once the metadata database is reachable.',
93+
),
94+
{ code: 'SERVICE_UNAVAILABLE', status: 503 },
95+
);
96+
}
97+
98+
function mount(svc: Svc, options: any = {}) {
99+
const routes = new Map<string, RouteHandler>();
100+
const server = {
101+
get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); },
102+
post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); },
103+
put: () => {},
104+
delete: (p: string, h: RouteHandler) => { routes.set(`DELETE:${p}`, h); },
105+
patch: () => {},
106+
use: () => {},
107+
listen: async () => {},
108+
close: async () => {},
109+
} as any;
110+
// The authorization gate (#7033 / #7023) is not this file's subject, so the
111+
// caller is stubbed holding the ADR-0106 D4 read set.
112+
registerPackageRoutes(server, () => svc as any, '/api/v1', {
113+
resolveExecutionContext: async () => ({
114+
userId: 'u_pkg', systemPermissions: ['manage_metadata', 'studio.access', 'setup.access'],
115+
}),
116+
...options,
117+
});
118+
return routes;
119+
}
120+
121+
async function drive(
122+
routes: Map<string, RouteHandler>,
123+
method: string,
124+
path: string,
125+
req: Record<string, any> = {},
126+
): Promise<Captured> {
127+
const handler = routes.get(`${method}:${path}`);
128+
if (!handler) throw new Error(`no handler for ${method} ${path}`);
129+
const captured: Captured = { status: 200, body: undefined };
130+
const res: any = {
131+
json(data: any) { captured.body = data; },
132+
send() {},
133+
status(code: number) { captured.status = code; return res; },
134+
header() { return res; },
135+
};
136+
await handler(
137+
{ params: {}, query: {}, body: undefined, headers: {}, method, path, ...req } as any,
138+
res,
139+
);
140+
return captured;
141+
}
142+
143+
const DB_MANIFEST = { id: 'com.acme.published', version: '2.0.0' };
144+
145+
/** A durable half that DOES answer — so a swallowed registry failure has
146+
* something to answer 200 with, exactly as the defect did. */
147+
const DURABLE_ROWS: Svc = {
148+
list: async () => [{ id: 'com.acme.published', version: '2.0.0', manifest: DB_MANIFEST }],
149+
};
150+
151+
/** A registry half whose PRESENT `getMetaItems` refuses with the declared 503. */
152+
const REFUSING_REGISTRY = {
153+
protocol: { getMetaItems: async () => { throw metadataStoreUnavailable(); } },
154+
};
155+
156+
describe('#11130 GET /packages — a failed REGISTRY read reaches the client', () => {
157+
it('answers the producer’s declared refusal (503 SERVICE_UNAVAILABLE), not a 200', async () => {
158+
const { status, body } = await drive(mount(DURABLE_ROWS, REFUSING_REGISTRY), 'GET', PKGS);
159+
160+
// code AND status — the ADR-0112 envelope, never a bare `toThrow()` and
161+
// never a status on its own.
162+
expect(status).toBe(503);
163+
expect(body.success).toBe(false);
164+
expect(body.error.code).toBe('SERVICE_UNAVAILABLE');
165+
166+
// …carried in the DECLARED envelope, not an ad-hoc body.
167+
expect(BaseResponseSchema.safeParse(body).success, JSON.stringify(body)).toBe(true);
168+
expect(envelopeViolations(body), JSON.stringify(body)).toEqual([]);
169+
expect(typeof body.error.message).toBe('string');
170+
expect(body.error.message.length).toBeGreaterThan(0);
171+
});
172+
173+
it('reports NO `total` over a read that failed — the corrupted complete count is gone', async () => {
174+
const { status, body } = await drive(mount(DURABLE_ROWS, REFUSING_REGISTRY), 'GET', PKGS);
175+
176+
// The defect's signature: a `total` presented as a complete count while the
177+
// registry half was missing, and a `packages` array the caller could not
178+
// tell apart from a full listing.
179+
expect(status).not.toBe(200);
180+
expect(body.data?.total).toBeUndefined();
181+
expect(body.data?.packages).toBeUndefined();
182+
183+
// And specifically NOT the database-only listing served as if it were whole.
184+
expect(body.data?.packages).not.toEqual([
185+
expect.objectContaining({ source: 'database' }),
186+
]);
187+
});
188+
189+
it('answers the SAME failure identically whichever HALF of the merge refuses', async () => {
190+
// One door, two sources. #11063 made the durable half answer the producer's
191+
// refusal; a registry half that still swallowed meant the SAME outage got
192+
// two different answers depending on which store was down. Agreement is the
193+
// fix, and it is worth one assertion.
194+
const registryHalf = await drive(mount(DURABLE_ROWS, REFUSING_REGISTRY), 'GET', PKGS);
195+
const durableHalf = await drive(
196+
mount(
197+
{ list: async () => { throw metadataStoreUnavailable(); } },
198+
{ protocol: { getMetaItems: async () => ({ items: [] }) } },
199+
),
200+
'GET',
201+
PKGS,
202+
);
203+
204+
expect(registryHalf.status).toBe(durableHalf.status);
205+
expect(registryHalf.body.error.code).toBe(durableHalf.body.error.code);
206+
expect(registryHalf.body.success).toBe(durableHalf.body.success);
207+
});
208+
209+
it('an UNDECLARED throw from the registry read is a 500 INTERNAL_ERROR, not a 200', async () => {
210+
// The other half of "stop absorbing": a throw carrying no declared envelope
211+
// is a server fault and now reaches the outer catch. Before this change the
212+
// arm was unreachable on this source — the bare `catch {}` ate it too.
213+
const { status, body } = await drive(
214+
mount(DURABLE_ROWS, {
215+
protocol: { getMetaItems: async () => { throw new Error('registry exploded'); } },
216+
}),
217+
'GET',
218+
PKGS,
219+
);
220+
221+
expect(status).toBe(500);
222+
expect(body.success).toBe(false);
223+
expect(body.error.code).toBe('INTERNAL_ERROR');
224+
expect(envelopeViolations(body), JSON.stringify(body)).toEqual([]);
225+
});
226+
227+
it('an ABSENT protocol service is still the different, HANDLED case — 200, database only', async () => {
228+
// The overreach guard. The `if (options.protocol && typeof … === 'function')`
229+
// guard answers "no registry here", which is a fact, not a failed read; a
230+
// fix that turned a composition without the protocol service into a 503
231+
// would break every such deployment. Nothing about that path moved.
232+
const { status, body } = await drive(mount(DURABLE_ROWS, {}), 'GET', PKGS);
233+
234+
expect(status).toBe(200);
235+
expect(body.success).toBe(true);
236+
expect(body.data.total).toBe(1);
237+
expect(body.data.packages[0].source).toBe('database');
238+
});
239+
240+
it('a registry read that ANSWERS still merges both sources and counts them truthfully', async () => {
241+
// The half that keeps this from being "refuse always": nothing about the
242+
// healthy path moved. Two sources, one overlapping id, and a `total` that is
243+
// a real complete count of what was really read.
244+
const { status, body } = await drive(
245+
mount(
246+
{
247+
list: async () => [
248+
{ id: 'com.acme.published', version: '2.0.0', manifest: DB_MANIFEST },
249+
{ id: 'com.acme.both', version: '1.0.0', manifest: { id: 'com.acme.both' } },
250+
],
251+
},
252+
{
253+
protocol: {
254+
getMetaItems: async () => ({
255+
items: [
256+
{ manifest: { id: 'com.acme.both', version: '1.0.0' } },
257+
{ manifest: { id: 'com.acme.registry-only', version: '1.0.0' } },
258+
],
259+
}),
260+
},
261+
},
262+
),
263+
'GET',
264+
PKGS,
265+
);
266+
267+
expect(status).toBe(200);
268+
expect(body.success).toBe(true);
269+
expect(body.data.total).toBe(3);
270+
expect(body.data.packages).toHaveLength(3);
271+
272+
const bySource = Object.fromEntries(
273+
body.data.packages.map((p: any) => [p.manifest?.id ?? p.id, p.source]),
274+
);
275+
expect(bySource['com.acme.registry-only']).toBe('registry');
276+
expect(bySource['com.acme.published']).toBe('database');
277+
expect(bySource['com.acme.both']).toBe('both');
278+
});
279+
});

0 commit comments

Comments
 (0)