fix(service-datasource): the datasource-admin 503 names the service the route actually needs (#4225) (#4234)

Six of the nine service-backed routes in admin-routes.ts resolve
`datasource-admin`; three resolve `external-datasource` and still answered
"The datasource-admin service is not available." An operator whose federation
service was unwired was sent to a service that was running fine.

The lookup and the message now come from one argument: the two resolvers
collapse into `resolve(res, service, method)`, which answers the 503 itself
naming whatever service it failed to resolve. The per-route capability check
is preserved.

Wire-visible on those three routes only — `error.message` now matches the
string packages/rest/src/external-datasource-routes.ts already emits. Status
and `error.code` unchanged on all nine.

All nine 503s are pinned to the service they name, driven against a context
that resolves services per name (the old mock answered the same object for
every lookup, which is why nothing could see this).

Closes #4225

Author: os-zhuang

.changeset/datasource-admin-503-names-its-own-service.md

@@ -0,0 +1,57 @@
+---
+"@objectstack/service-datasource": patch
+---
+
+fix(service-datasource): the datasource-admin 503 names the service the route actually needs (#4225)
+
+`admin-routes.ts` registered nine service-backed routes behind one hard-coded 503:
+
+```ts
+const unavailable = (res) =>
+  sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The datasource-admin service is not available.');
+```
+
+Six of those routes resolve `datasource-admin`, so the message was right. Three
+resolve `external-datasource` — `GET /:name/remote-tables`, `POST /:name/test`,
+`POST /:name/object-draft` — and answered with the same sentence. An operator
+whose federation service was unwired was told to go look at `datasource-admin`,
+which was running fine.
+
+The code was never the bug. `SERVICE_UNAVAILABLE` is correct for all nine:
+ADR-0112's ledger asks generic conditions to reuse the standard catalog rather
+than register a per-service 503 synonym, and this module documents that decision
+inline. Which service is down is carried by `message`, exactly as intended — the
+`message` was simply wrong on three routes.
+
+Rather than parameterise the 503 helper and leave the name typed out a second
+time at each call site, the lookup and the message now come from one argument.
+The two `adminService()` / `externalService()` resolvers collapse into a single
+`resolve(res, service, method)` that answers the 503 itself, naming whatever
+service it just failed to resolve:
+
+```ts
+const svc = resolve(res, 'external-datasource', 'listRemoteTables');
+if (!svc) return;
+```
+
+Fixing the three messages needed only the parameter; taking the name from the
+lookup is what stops a tenth route reintroducing the mismatch. The per-route
+capability check is preserved — a host may wire a partial implementation, so
+"the service is registered" and "this route can use it" stay separate facts.
+
+Wire-visible change, on those three routes only: the 503 body's `error.message`
+now reads `The external-datasource service is not available.` — the same string
+`packages/rest/src/external-datasource-routes.ts` already emits for its own
+surface. Status and `error.code` are unchanged on all nine.
+
+Each of the nine 503s is now pinned to the service it names, driven through the
+real `HonoHttpServer` against a context that resolves services **per name**. The
+mock every existing test used answers the same object for every lookup, which is
+why nothing could see this: it cannot tell the two services apart. One case
+covers the operator's actual situation — `datasource-admin` wired and answering
+200s, `external-datasource` absent — including `POST /:name/test`, where the
+wired admin service has a `testConnection` of its own and must not answer for the
+external route.
+
+Pre-existing: #3843 carried every code string over verbatim and #3973 changed no
+bytes on the wire.

packages/services/service-datasource/README.md

@@ -37,7 +37,10 @@ The runtime admin owns only the `origin: 'runtime'` lifecycle.
 
 Mounted under `/api/v1/datasources` by `registerDatasourceAdminRoutes` (lifecycle
 + introspection) and the federation routes by the external service. Every route
-degrades gracefully (`503` / "unavailable") when its service isn't wired.
+degrades gracefully (`503` / "unavailable") when its service isn't wired, and the
+message names **that** service — one registrar, but two services behind it: the
+three routes marked below dispatch to `external-datasource`, the rest to
+`datasource-admin` (#4225).
 
 **Lifecycle & connection**
 - `GET    /datasources` — list (code + runtime, with provenance/health)
@@ -47,9 +50,9 @@ degrades gracefully (`503` / "unavailable") when its service isn't wired.
 - `PATCH  /datasources/:name` — update a runtime datasource
 - `DELETE /datasources/:name` — remove a runtime datasource (blocked while objects are bound)
 - `POST   /datasources/test` — probe an unsaved draft (inline body)
-- `POST   /datasources/:name/test` — probe a **saved** datasource by name (backs the `test_connection` action)
+- `POST   /datasources/:name/test` — probe a **saved** datasource by name (backs the `test_connection` action) — *`external-datasource`*
 
-**Introspection / sync (read-only)**
+**Introspection / sync (read-only)** — all on `external-datasource`
 - `GET    /datasources/:name/remote-tables` — list remote tables
 - `POST   /datasources/:name/object-draft` — generate an object definition draft for one table (no persistence)
 - federation import/validate/refresh routes under `/datasources/:name/external/*` (ADR-0015)

packages/services/service-datasource/src/__tests__/admin-routes.test.ts

@@ -26,6 +26,22 @@ function mount(svc: unknown) {
   return server.getRawApp();
 }
 
+/**
+ * Mount with a service PER NAME, unlike `mount` above, which answers the same
+ * object for every lookup.
+ *
+ * That difference is the point: this module dispatches to two services, and a
+ * context that cannot tell them apart cannot show which one a route resolved.
+ * It is what let #4225 sit here — the 503 named `datasource-admin` on all nine
+ * routes, three of which resolve `external-datasource`, and no test could see it.
+ */
+function mountServices(services: Record<string, unknown>) {
+  const server = new HonoHttpServer(0);
+  const ctx = { getService: vi.fn((name: string) => services[name]) } as any;
+  registerDatasourceAdminRoutes(server, ctx, '/api/v1');
+  return server.getRawApp();
+}
+
 describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => {
   it('GET /api/v1/datasources returns the service listing', async () => {
     const listDatasources = vi.fn().mockResolvedValue([
@@ -136,10 +152,19 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => {
     expect(await res.json()).toEqual({ success: true, data: { datasource: { name: 'pg', origin: 'runtime' } } });
   });
 
-  it('degrades to 503 when the datasource-admin service is not wired', async () => {
-    const app = mount(undefined);
+  it('degrades to 503 when the service registry THROWS, not just when it answers undefined', async () => {
+    // The other arm of the resolver's try/catch. `getService` throwing on an
+    // unregistered name is the shape a real `PluginContext` has; every mock in
+    // this file returns `undefined` instead, so nothing else drives this branch.
+    const server = new HonoHttpServer(0);
+    const ctx = {
+      getService: vi.fn(() => {
+        throw new Error('service "datasource-admin" is not registered');
+      }),
+    } as any;
+    registerDatasourceAdminRoutes(server, ctx, '/api/v1');
 
-    const res = await app.fetch(json('/api/v1/datasources'));
+    const res = await server.getRawApp().fetch(json('/api/v1/datasources'));
 
     expect(res.status).toBe(503);
     expect(await res.json()).toEqual({
@@ -151,6 +176,69 @@ describe('registerDatasourceAdminRoutes (real HonoHttpServer)', () => {
     });
   });
 
+  /**
+   * #4225 — the 503 names the service the route ACTUALLY resolves.
+   *
+   * Every route below is listed with the service it dispatches to, so the table
+   * is the module's service map as well as its test: a new route that resolves
+   * one service and reports the other has to disagree with a row here.
+   */
+  const UNAVAILABLE: Array<{ route: string; service: string; run: (app: any) => Promise<Response> }> = [
+    { route: 'GET /datasources', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources')) },
+    { route: 'GET /datasources/:name', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/pg')) },
+    { route: 'POST /datasources/test', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/test', { method: 'POST', body: '{}' })) },
+    { route: 'POST /datasources', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources', { method: 'POST', body: '{}' })) },
+    { route: 'PATCH /datasources/:name', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/pg', { method: 'PATCH', body: '{}' })) },
+    { route: 'DELETE /datasources/:name', service: 'datasource-admin', run: (a) => a.fetch(json('/api/v1/datasources/pg', { method: 'DELETE' })) },
+    { route: 'GET /datasources/:name/remote-tables', service: 'external-datasource', run: (a) => a.fetch(json('/api/v1/datasources/ext/remote-tables')) },
+    { route: 'POST /datasources/:name/test', service: 'external-datasource', run: (a) => a.fetch(json('/api/v1/datasources/ext/test', { method: 'POST', body: '{}' })) },
+    { route: 'POST /datasources/:name/object-draft', service: 'external-datasource', run: (a) => a.fetch(json('/api/v1/datasources/ext/object-draft', { method: 'POST', body: JSON.stringify({ table: 'customers' }) })) },
+  ];
+
+  for (const c of UNAVAILABLE) {
+    it(`${c.route} degrades to 503 naming ${c.service} (#4225)`, async () => {
+      const res = await c.run(mountServices({}));
+      expect(res.status).toBe(503);
+      expect(await res.json()).toEqual({
+        success: false,
+        error: {
+          code: 'SERVICE_UNAVAILABLE',
+          message: `The ${c.service} service is not available.`,
+        },
+      });
+    });
+  }
+
+  it('a wired datasource-admin does not answer for an unwired external-datasource (#4225)', async () => {
+    // The operator's actual situation: lifecycle works, federation does not. The
+    // old message sent them to read the logs of the service that was running.
+    const app = mountServices({
+      'datasource-admin': {
+        listDatasources: async () => [{ name: 'ext', origin: 'runtime' }],
+        getDatasource: async () => ({ name: 'ext', driver: 'sqlite' }),
+        testConnection: async () => ({ ok: true }),
+      },
+      // 'external-datasource' deliberately absent.
+    });
+
+    expect((await app.fetch(json('/api/v1/datasources'))).status).toBe(200);
+
+    const remote = await app.fetch(json('/api/v1/datasources/ext/remote-tables'));
+    expect(remote.status).toBe(503);
+    expect(((await remote.json()) as any).error.message).toBe(
+      'The external-datasource service is not available.',
+    );
+
+    // `POST /:name/test` resolves `external-datasource` even though its unsaved-draft
+    // sibling `POST /test` resolves `datasource-admin` — the wired admin service
+    // above has a `testConnection`, and it must not answer for this route.
+    const saved = await app.fetch(json('/api/v1/datasources/ext/test', { method: 'POST', body: '{}' }));
+    expect(saved.status).toBe(503);
+    expect(((await saved.json()) as any).error.message).toBe(
+      'The external-datasource service is not available.',
+    );
+  });
+
   it('surfaces lifecycle errors as 400 with the service message', async () => {
     const createDatasource = vi.fn().mockRejectedValue(new Error('duplicate name'));
     const app = mount({ createDatasource });

packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts

@@ -172,6 +172,15 @@ describe('datasource-admin envelope (#3843) — error bodies', () => {
       code: 'SERVICE_UNAVAILABLE',
       run: () => drive(mount(undefined), '/api/v1/datasources'),
     },
+    {
+      // The same 503, from the three routes served by the OTHER service (#4225).
+      // Which service is named is asserted in `admin-routes.test.ts`; what this
+      // row adds is that the branch emits the declared envelope, like its twin.
+      name: 'the external-datasource service is not wired',
+      status: 503,
+      code: 'SERVICE_UNAVAILABLE',
+      run: () => drive(mount(undefined), '/api/v1/datasources/ext/remote-tables'),
+    },
     {
       name: 'a lifecycle failure carries the service message',
       status: 400,