fix(service-datasource)!: a datasource update whose new configuration fails to connect stores none of it

.changeset/29879-datasource-update-kept-pool-stores-nothing.md

@@ -0,0 +1,18 @@
+---
+'@objectstack/service-datasource': patch
+---
+
+fix(service-datasource)!: a datasource update whose new configuration fails to connect stores none of it, and a whitespace-only `secret` is refused like an empty one
+
+Clause-②: yes (narrowing) — `PATCH /api/v1/datasources/:name` and `POST /api/v1/datasources` refuse one more body that used to answer 200 / 201: a `secret` whose value is whitespace only (`"   "`, a tab and a newline), now `400 DATASOURCE_ADMIN_ERROR` with the empty-secret answer. The widening half: the `reregisterPool` member of the exported `DatasourceAdminServiceConfig` may now answer `{ previousPoolKept: boolean }` (still optional; a seam that answers nothing reads as before).
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped: secret is the doors' own credential channel, split off the body before the datasource draft is judged. A blank value seals nothing usable, so there is nothing at rest for a migration to rewrite. -->
+
+**BREAKING** accept-set narrowing at the datasource admin doors (`@objectstack/service-datasource`): a whitespace-only `secret` is refused.
+
+- **Before.** A `PATCH` whose new configuration failed to connect (a wrong password, a blank `secret`, a changed `config` that does not reach the database) answered `200` with `status: "error"` and a reason saying the previous pool was kept and the new configuration was NOT in force. It had already stored the new record, repointed `external.credentialsRef` at a new `sys_secret` row and removed the working one, so the next restart connected the configuration that failed. A whitespace-only `secret` passed the empty-secret guard and reached this path.
+- **Now.** The update rebuilds the pool before it stores anything. When the previous pool is kept, nothing of the patch is stored: the record, its `credentialsRef` and every other key of the patch stay as they were, and the `sys_secret` row sealed for the patch is removed again. The answer is unchanged (`200`, `status: "error"`, "the new configuration is NOT in force"), and now true: a restart connects the configuration that was serving. A configuration that connects is stored and in force, and the replaced secret is retired, as before. A whitespace-only `secret` is refused before anything is written, by the same guard and with the same message as the empty string; a value with whitespace around other characters is still sealed as sent.
+- **Who moves.** The console sends `secret` only when the field holds a value. A script or SDK call that sends a whitespace-only `secret` gets the 400. A host that wires its own `reregisterPool` seam keeps today's behaviour until it answers `{ previousPoolKept: true }` for a rebuild that kept the previous pool.
+
+**The remedy.** To keep the stored credential, leave `secret` out of the request (or send `null`). To replace it, send the new credential.

packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts

@@ -30,6 +30,9 @@ import {
  * is written, the stored record and its `credentialsRef` are unchanged, and no
  * secret is removed. `null`, an absent `secret` and a real one are unchanged.
  *
+ * #29879 — a whitespace-only `secret` (`"   "`, a tab and a newline) is blank
+ * the same way, and the same guard refuses it with the same answer.
+ *
  * Driven through the REAL routes over the REAL service on in-memory stores
  * (the harness of `datasource-admin-declared-keys-kept.test.ts`). Every
  * secret below is an obvious placeholder.
@@ -135,11 +138,18 @@ async function expectSecretRefusal(res: Response): Promise<string> {
     code: 'DATASOURCE_ADMIN_ERROR',
   });
   expect(body.error!.message).toContain('`secret`');
+  expect(body.error!.message).toContain('Nothing was written.');
   return body.error!.message;
 }
 
 describe('PATCH /api/v1/datasources/:name — an empty secret (#28928)', () => {
-  for (const [label, secret] of [['an empty string', ''], ['an empty `value` object', { value: '' }]] as const) {
+  for (const [label, secret] of [
+    ['an empty string', ''],
+    ['an empty `value` object', { value: '' }],
+    // [#29879] Blank is empty: the same guard, the same answer.
+    ['a whitespace-only string', '   '],
+    ['a whitespace-only `value` object', { value: '\t\n' }],
+  ] as const) {
     it(`${label} is refused 400 naming \`secret\`; the credential, its ref and the record are unchanged`, async () => {
       const { send, records, ops, written } = makeDoor([STORED]);
       await expectSecretRefusal(await send('PATCH', '/datasources/wh', { secret }));
@@ -149,6 +159,12 @@ describe('PATCH /api/v1/datasources/:name — an empty secret (#28928)', () => {
     });
   }
 
+  it('control: a secret with whitespace around other characters is a value, sealed as sent (#29879)', async () => {
+    const { send, written } = makeDoor([STORED]);
+    expect((await send('PATCH', '/datasources/wh', { secret: ` ${PLACEHOLDER_SECRET}\t` })).status).toBe(200);
+    expect(written).toEqual([` ${PLACEHOLDER_SECRET}\t`]);
+  });
+
   it('control: a real secret still rotates (a new ref, the old one removed)', async () => {
     const { send, records, ops, written } = makeDoor([STORED]);
     expect((await send('PATCH', '/datasources/wh', { secret: PLACEHOLDER_SECRET })).status).toBe(200);
@@ -169,13 +185,15 @@ describe('PATCH /api/v1/datasources/:name — an empty secret (#28928)', () => {
 });
 
 describe('POST /api/v1/datasources — an empty secret (#28928)', () => {
-  it('is refused 400 naming `secret`; nothing is stored and no secret is written', async () => {
-    const { send, records, ops, written } = makeDoor();
-    await expectSecretRefusal(await send('POST', '/datasources', { name: 'wh', ...BASE, secret: '' }));
-    expect(records.has('wh')).toBe(false);
-    expect(written).toEqual([]);
-    expect(ops).toEqual([]);
-  });
+  for (const secret of ['', '   ', '\t\n']) {
+    it(`${JSON.stringify(secret)} is refused 400 naming \`secret\`; nothing is stored and no secret is written`, async () => {
+      const { send, records, ops, written } = makeDoor();
+      await expectSecretRefusal(await send('POST', '/datasources', { name: 'wh', ...BASE, secret }));
+      expect(records.has('wh')).toBe(false);
+      expect(written).toEqual([]);
+      expect(ops).toEqual([]);
+    });
+  }
 
   it('control: a real secret is sealed and bound', async () => {
     const { send, records, written } = makeDoor();

packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts

@@ -0,0 +1,314 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * #29879 — a datasource PATCH whose new configuration fails to connect
+ * answers that the previous pool was kept and the new configuration is NOT in
+ * force; nothing of the new configuration may then be stored, so the next
+ * restart connects the configuration that was serving.
+ *
+ * The defect: `updateDatasource` stored the merged record, repointed
+ * `external.credentialsRef` at the newly sealed secret and removed the
+ * working one BEFORE the rebuild. The rebuild failed and the previous pool
+ * kept serving, so the answer read "nothing changed", but the restart read
+ * the failing credential and the datasource could no longer connect, with
+ * the working credential already gone.
+ *
+ * The rig boots the REAL `DatasourceAdminServicePlugin` and
+ * `ExternalDatasourceServicePlugin` behind the REAL admin routes, with the
+ * REAL `sys_secret` binder, over one in-memory store (`sys_metadata` and
+ * `sys_secret`) that outlives a boot. A restart is a second boot over the
+ * same store: fresh registries, fresh pools, the stored rows only. The
+ * driver factory's pool connects only with the database's real password,
+ * the one fact a wrong secret changes.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { HonoHttpServer } from '@objectstack/plugin-hono-server';
+import {
+  assertEngineDeleteDispatch,
+  assertEngineFindOnePredicate,
+  assertEngineUpdateDispatch,
+} from '@objectstack/metadata-core';
+import type { CryptoContext, CryptoHandle, ICryptoProvider, IDataDriver } from '@objectstack/spec/contracts';
+import { DatasourceAdminServicePlugin } from '../datasource-admin-plugin.js';
+import { ExternalDatasourceServicePlugin } from '../plugin.js';
+import { registerDatasourceAdminRoutes } from '../admin-routes.js';
+import { createDatasourceSecretBinder } from '../datasource-secret-binder.js';
+import type { IDatasourceDriverFactory, DatasourceConnectionSpec } from '../contracts/datasource-driver-factory.js';
+import { ENTITLED_CREDENTIAL, createSessionAuthService, createGrantsEngine } from './entitled-caller.fixture.js';
+
+type Row = Record<string, unknown>;
+
+/** The database's real password: the only secret its pool connects with. */
+const REAL_PASSWORD = 'placeholder-real-password';
+const WRONG_PASSWORD = 'placeholder-wrong-password';
+const NEW_PASSWORD = 'placeholder-new-password';
+
+/** The store a restart keeps: `sys_metadata` and `sys_secret`, by table. */
+function makeStore() {
+  return { tables: new Map<string, Row[]>(), seq: 0 };
+}
+type Store = ReturnType<typeof makeStore>;
+
+function rowsOf(store: Store, table: string): Row[] {
+  if (!store.tables.has(table)) store.tables.set(table, []);
+  return store.tables.get(table)!;
+}
+
+function matches(row: Row, where: Record<string, unknown>): boolean {
+  for (const [k, v] of Object.entries(where)) {
+    if (k.startsWith('$')) throw new Error(`fake engine: unsupported operator ${k}`);
+    if (v === undefined) continue;
+    if (row[k] !== v) return false;
+  }
+  return true;
+}
+
+/** AAD-binding crypto double, a fresh handle id per seal (as the shipped provider mints). */
+function fakeCrypto(store: Store): ICryptoProvider {
+  const aadOf = (ctx: CryptoContext) => JSON.stringify([ctx.scope, ctx.namespace, ctx.key]);
+  return {
+    async encrypt(plain: string, ctx: CryptoContext): Promise<CryptoHandle> {
+      return {
+        id: `sec_${++store.seq}`,
+        kmsKeyId: 'local:test:v1',
+        alg: 'aes-256-gcm',
+        version: 1,
+        ciphertext: Buffer.from(JSON.stringify([aadOf(ctx), plain]), 'utf8').toString('base64'),
+      };
+    },
+    async decrypt(handle: CryptoHandle, ctx: CryptoContext): Promise<string> {
+      const [aad, plain] = JSON.parse(Buffer.from(handle.ciphertext, 'base64').toString('utf8'));
+      if (aad !== aadOf(ctx)) throw new Error('AAD mismatch');
+      return plain;
+    },
+    async rotateKey(handle: CryptoHandle): Promise<CryptoHandle> {
+      return handle;
+    },
+    digest: (plain: string) => `sha256:${plain.length}`,
+    keyedDigest: async (plain: string) => `k:${plain.length}`,
+  };
+}
+
+/** A pool: it connects only to database `qa` with the real password; its health is its connection. */
+function passwordFactory(): IDatasourceDriverFactory & { built: string[] } {
+  const built: string[] = [];
+  return {
+    built,
+    supports: () => true,
+    create: async (spec: DatasourceConnectionSpec) => {
+      built.push(spec.secret ?? '');
+      let connected = false;
+      const driver = {
+        name: 'com.fake.postgres',
+        checkHealth: async () => connected,
+        disconnect: async () => {
+          connected = false;
+        },
+      };
+      return {
+        driver: driver as unknown as IDataDriver,
+        connect: async () => {
+          if ((spec.config as { database?: unknown })?.database !== 'qa') throw new Error('database does not exist');
+          if (spec.secret !== REAL_PASSWORD) throw new Error('password authentication failed');
+          connected = true;
+        },
+      };
+    },
+  };
+}
+
+/** One boot over `store`: the real plugins, the real routes, a driver registry of its own. */
+async function boot(store: Store) {
+  const drivers = new Map<string, IDataDriver>();
+  const data = {
+    findOne: async (table: string, q: { where: Record<string, unknown> }) => {
+      assertEngineFindOnePredicate(table, q);
+      return rowsOf(store, table).find((r) => matches(r, q.where)) ?? null;
+    },
+    find: async (table: string, q?: { where?: Record<string, unknown>; limit?: number }) => {
+      const all = rowsOf(store, table).filter((r) => matches(r, q?.where ?? {}));
+      return typeof q?.limit === 'number' ? all.slice(0, q.limit) : all;
+    },
+    insert: async (table: string, row: Row) => {
+      const stored = { ...row, id: row.id ?? `r_${++store.seq}` };
+      rowsOf(store, table).push(stored);
+      return { id: stored.id };
+    },
+    update: async (table: string, row: Row, q: { where: Record<string, unknown> }) => {
+      assertEngineUpdateDispatch(row, q);
+      for (const r of rowsOf(store, table)) if (matches(r, q.where)) Object.assign(r, row);
+      return {};
+    },
+    delete: async (table: string, q: { where: Record<string, unknown> }) => {
+      assertEngineDeleteDispatch(q);
+      const rows = rowsOf(store, table);
+      const before = rows.length;
+      for (let i = rows.length - 1; i >= 0; i--) if (matches(rows[i], q.where)) rows.splice(i, 1);
+      return { deleted: before - rows.length };
+    },
+    registerDriver: (driver: IDataDriver) => {
+      if (!drivers.has(driver.name)) drivers.set(driver.name, driver);
+    },
+    unregisterDriver: (name: string) => drivers.delete(name),
+    getDriverByName: (name: string) => drivers.get(name),
+    registerDatasourceDef: () => {},
+    markDatasourceUnavailable: () => {},
+    clearDatasourceUnavailable: () => {},
+  };
+
+  const registry = new Map<string, Map<string, unknown>>();
+  const metadata = {
+    get: async (t: string, n: string) => registry.get(t)?.get(n),
+    list: async (t: string) => [...(registry.get(t)?.values() ?? [])],
+    register: async (t: string, n: string, d: unknown) => {
+      if (!registry.has(t)) registry.set(t, new Map());
+      registry.get(t)!.set(n, d);
+    },
+    unregister: async (t: string, n: string) => {
+      registry.get(t)?.delete(n);
+    },
+    listObjects: async () => [],
+  };
+
+  const services = new Map<string, unknown>([
+    ['data', data],
+    ['metadata', metadata],
+    ['auth', createSessionAuthService()],
+    // The routes resolve the caller's grants from `objectql` first.
+    ['objectql', createGrantsEngine()],
+  ]);
+  const logger = { warn: vi.fn(), info: vi.fn(), debug: vi.fn(), error: vi.fn() };
+  const ctx: any = {
+    getService: (name: string) => {
+      if (services.has(name)) return services.get(name);
+      throw new Error(`no service ${name}`);
+    },
+    registerService: (name: string, svc: unknown) => {
+      services.set(name, svc);
+    },
+    trigger: async () => {},
+    logger,
+  };
+
+  const secrets = createDatasourceSecretBinder({ engine: data as never, cryptoProvider: fakeCrypto(store) });
+  const factory = passwordFactory();
+  const admin = new DatasourceAdminServicePlugin({ driverFactory: factory, secrets, logger });
+  const external = new ExternalDatasourceServicePlugin();
+  await admin.init(ctx);
+  await external.init(ctx);
+  await admin.start(ctx);
+  await external.start(ctx);
+
+  const server = new HonoHttpServer(0);
+  registerDatasourceAdminRoutes(server, ctx, '/api/v1');
+  const app = server.getRawApp();
+  const send = async (method: string, path: string, body?: unknown) => {
+    const res = await app.fetch(
+      new Request(`http://local/api/v1${path}`, {
+        method,
+        headers: { 'content-type': 'application/json', authorization: ENTITLED_CREDENTIAL },
+        ...(body === undefined ? {} : { body: JSON.stringify(body) }),
+      }),
+    );
+    return { status: res.status, body: (await res.json()) as any };
+  };
+  return { send, factory, logger };
+}
+
+const CREATE = {
+  name: 'r7pg3',
+  driver: 'postgres',
+  schemaMode: 'external',
+  external: {},
+  config: { host: '127.0.0.1', port: 5432, database: 'qa', username: 'qa' },
+};
+
+/** The stored datasource record and the `sys_secret` ids, as a restart will read them. */
+function atRest(store: Store) {
+  const row = rowsOf(store, 'sys_metadata').find((r) => r.type === 'datasource' && r.name === CREATE.name);
+  return {
+    record: row ? JSON.parse(String(row.metadata)) : undefined,
+    secretIds: rowsOf(store, 'sys_secret').map((r) => r.id).sort(),
+  };
+}
+
+async function created(store: Store) {
+  const first = await boot(store);
+  const res = await first.send('POST', '/datasources', { ...CREATE, secret: REAL_PASSWORD });
+  expect({ status: res.status, dsStatus: res.body.data?.datasource?.status }).toEqual({ status: 201, dsStatus: 'ok' });
+  return first;
+}
+
+describe('PATCH /api/v1/datasources/:name — a new secret that fails to connect (#29879)', () => {
+  it('answers NOT in force, stores nothing of it, and the restart still connects', async () => {
+    const store = makeStore();
+    const first = await created(store);
+    const before = atRest(store);
+    expect(before.record.external.credentialsRef).toMatch(/^sys_secret:/);
+    expect(before.secretIds).toHaveLength(1);
+
+    const res = await first.send('PATCH', `/datasources/${CREATE.name}`, {
+      secret: WRONG_PASSWORD,
+      label: 'Renamed in the same patch',
+    });
+    expect(res.status).toBe(200);
+    expect(res.body.data.datasource.status).toBe('error');
+    expect(res.body.data.datasource.statusReason).toContain('the new configuration is NOT in force');
+
+    // Nothing of the patch is at rest: not the new binding, not the other key,
+    // and the secret sealed for it is gone again.
+    expect(atRest(store)).toEqual(before);
+    // The kept pool still serves.
+    expect((await first.send('POST', `/datasources/${CREATE.name}/test`)).body.data).toMatchObject({ ok: true });
+
+    const restarted = await boot(store);
+    expect((await restarted.send('POST', `/datasources/${CREATE.name}/test`)).body.data).toMatchObject({ ok: true });
+    expect(restarted.factory.built).toEqual([REAL_PASSWORD]);
+  });
+
+  it('the same for a failing config change: the stored config is the one serving', async () => {
+    const store = makeStore();
+    const first = await created(store);
+    const before = atRest(store);
+
+    const res = await first.send('PATCH', `/datasources/${CREATE.name}`, {
+      config: { ...CREATE.config, database: 'qa_other' },
+    });
+    expect(res.body.data.datasource.statusReason).toContain('the new configuration is NOT in force');
+    expect(atRest(store)).toEqual(before);
+    expect((await boot(store)).factory.built).toEqual([REAL_PASSWORD]);
+  });
+
+  it('control: a secret that connects is in force and stored, and the one it replaces is retired', async () => {
+    const store = makeStore();
+    const first = await created(store);
+    const before = atRest(store);
+
+    const res = await first.send('PATCH', `/datasources/${CREATE.name}`, { secret: REAL_PASSWORD });
+    expect(res.status).toBe(200);
+    expect(res.body.data.datasource.status).toBe('ok');
+
+    const after = atRest(store);
+    expect(after.record.external.credentialsRef).not.toBe(before.record.external.credentialsRef);
+    expect(after.secretIds).toHaveLength(1);
+    expect(after.secretIds).not.toEqual(before.secretIds);
+
+    const restarted = await boot(store);
+    expect((await restarted.send('POST', `/datasources/${CREATE.name}/test`)).body.data).toMatchObject({ ok: true });
+  });
+
+  it('a second failing PATCH after a first stores nothing either', async () => {
+    // The first leaves the previous pool serving under a FAILED verdict, so
+    // "kept" cannot be read off the verdict the rebuild started from.
+    const store = makeStore();
+    const first = await created(store);
+    const before = atRest(store);
+    for (const secret of [WRONG_PASSWORD, NEW_PASSWORD]) {
+      const res = await first.send('PATCH', `/datasources/${CREATE.name}`, { secret });
+      expect(res.body.data.datasource.statusReason).toContain('the new configuration is NOT in force');
+    }
+    expect(atRest(store)).toEqual(before);
+    expect((await boot(store)).factory.built).toEqual([REAL_PASSWORD]);
+  });
+});