From 0776e31bc8ee4a46ff284dadc3f98cb0094c17b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 20:48:33 +0000 Subject: [PATCH 1/4] fix(service-datasource)!: a datasource update whose rebuild keeps the previous pool stores none of the new configuration A PATCH whose new configuration failed to connect answered that the previous pool was kept and the new configuration was NOT in force, but had already stored the merged record, repointed external.credentialsRef at the new secret and removed the working one. The next restart connected the configuration that failed. The update now rebuilds the pool before it stores the record. When the reregisterPool seam reports previousPoolKept, nothing is stored and the secret sealed for the write is taken back out; the working secret is retired only after the merged record is stored. The plugin's seam reports the outcome from the state the rebuild left. The blank-secret guard also refuses a whitespace-only secret, with the same message. Claude-Session: https://claude.ai/code/session_01WGufzfDo764sexAMz4Feui Co-authored-by: Claude --- .../src/datasource-admin-plugin.ts | 37 +++++- .../src/datasource-admin-service.ts | 114 +++++++++++++++--- 2 files changed, 130 insertions(+), 21 deletions(-) diff --git a/packages/services/service-datasource/src/datasource-admin-plugin.ts b/packages/services/service-datasource/src/datasource-admin-plugin.ts index bdf7f31452b..bfcfd9b6b05 100644 --- a/packages/services/service-datasource/src/datasource-admin-plugin.ts +++ b/packages/services/service-datasource/src/datasource-admin-plugin.ts @@ -616,13 +616,40 @@ export class DatasourceAdminServicePlugin implements Plugin { // [#27750] The rebuild carries the objects bound to the NEW record, the // way `registerPool` does on create: a changed `config` can open a new, // empty database, and without them the bound objects had no table there. + // + // [#29879] It reports whether the previous pool was kept, which the + // update door reads to store none of a configuration that never came + // into force. The reading is the state the rebuild left: the new + // configuration is not available, yet a driver still answers the name — + // `reconnect` put the previous pool back, the only driver a failed + // rebuild leaves registered. The pool record follows the pool, so a + // later cluster convergence compares the shared row against the + // configuration actually serving. reregisterPool: async (previous, next) => { + const connection = this.connection; + if (!connection) return { previousPoolKept: false }; this.livePoolRecords.set(next.name, next); - await this.connection?.reconnect(next, { - objects: managedObjectsBoundTo(next), - previous, - context: { origin: next.origin ?? 'runtime', trigger: 'runtime-admin' }, - }); + let failure: { error: unknown } | undefined; + try { + await connection.reconnect(next, { + objects: managedObjectsBoundTo(next), + previous, + context: { origin: next.origin ?? 'runtime', trigger: 'runtime-admin' }, + }); + } catch (error) { + failure = { error }; + } + const engine = engineOf() as ConnectionEngineLike | undefined; + const previousPoolKept = + connection.getConnectionState(next.name)?.availability !== 'available' + && engine?.getDriverByName?.(next.name) !== undefined; + if (!previousPoolKept) { + if (failure) throw failure.error; + return { previousPoolKept }; + } + this.livePoolRecords.set(next.name, previous); + if (failure) logger?.warn(`reregisterPool('${next.name}') failed`, failure.error); + return { previousPoolKept }; }, // [#13805] The receive half of the datasource cluster bridge — see diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index 0854b5220be..cea697a42a5 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/packages/services/service-datasource/src/datasource-admin-service.ts @@ -269,6 +269,16 @@ export interface ProbeInput { timeoutMs?: number; } +/** + * [#29879] What a {@link DatasourceAdminServiceConfig.reregisterPool} seam + * reports about the rebuild it ran. `previousPoolKept: true` = the new + * configuration failed to come up and the pool built from the previous one is + * still the one serving, so the update must store none of the new one. + */ +export interface ReregisterPoolOutcome { + previousPoolKept: boolean; +} + /** * Injected dependencies. The plugin supplies real implementations backed by the * driver registry, `IMetadataService` (runtime store), and the secret store; @@ -370,8 +380,17 @@ export interface DatasourceAdminServiceConfig { * Optional: a host that wired only `registerPool` falls back to it, whose * idempotency guard retains the old pool — the pre-#13804 behaviour, and * the safe direction for a host that cannot express a swap. + * + * [#29879] Answer `{ previousPoolKept: true }` when the rebuild failed and + * the previous pool is the one still serving: the update then stores + * nothing of the new configuration and takes back the secret it sealed for + * it. Answering nothing (a host written before this) keeps the old reading: + * the new record is stored whatever the rebuild did. */ - reregisterPool?: (previous: StoredDatasource, next: StoredDatasource) => Promise | void; + reregisterPool?: ( + previous: StoredDatasource, + next: StoredDatasource, + ) => Promise | ReregisterPoolOutcome | void; /** * [#13805] Converge THIS replica's live pool for `name` after a PEER wrote * the shared datasource record — the receive half of @@ -490,11 +509,17 @@ export function datasourceNotFoundError(name: string): Error { * a script or an SDK call that clears the field is what reached it. * * `null` and an absent `secret` are unchanged: they keep the stored - * credential. A whitespace-only value is a value, sealed and used as sent. - * Only the empty string is judged, so the answer cannot echo a credential. + * credential. + * + * [#29879] A whitespace-only value (`" "`, a tab and a newline) is refused + * by this same guard: it seals nothing usable, and a rotation to it broke + * the datasource at its next connect exactly as the empty string did. A + * value with whitespace around other characters is a value, sealed as sent. + * Only blank values are judged, so the answer cannot echo a credential. */ function assertSecretNotEmpty(heading: string, secret: SecretInput | undefined): void { - if (secret == null || (secret as { value?: unknown }).value !== '') return; + const value = (secret as { value?: unknown } | null | undefined)?.value; + if (secret == null || typeof value !== 'string' || value.trim() !== '') return; throw new Error( `${heading}\nThe \`secret\` is empty. Send the credential to store, or leave \`secret\` out (or null) ` + 'to keep the stored one. Nothing was written.', @@ -952,11 +977,16 @@ export class DatasourceAdminService implements IDatasourceAdminService { // `active: false` unreachable. if (patch.external !== undefined) await this.assertConnectorLendable(merged); + // [#29879] The new secret is sealed BEFORE the rebuild (the rebuild + // dereferences the merged record's `credentialsRef`), but the working one + // is retired only once the merged record is stored, below: a rebuild that + // keeps the previous pool stores nothing, and the binding it keeps must + // still resolve. + const prevRef = existing.external?.credentialsRef; + let newRef: string | undefined; if (secret) { - const prevRef = existing.external?.credentialsRef; - const credentialsRef = await this.config.writeSecret(secret, { name }); - merged.external = { ...(merged.external ?? {}), credentialsRef }; - if (prevRef && prevRef !== credentialsRef) await this.tryRemoveSecret(prevRef); + newRef = await this.config.writeSecret(secret, { name }); + merged.external = { ...(merged.external ?? {}), credentialsRef: newRef }; } // Carry forward the credential material `getDatasource()` redacts (#8081) @@ -995,8 +1025,6 @@ export class DatasourceAdminService implements IDatasourceAdminService { merged.ssl = { ...merged.ssl, [SSL_PRIVATE_KEY]: storedKey }; } - await this.config.putDatasourceRecord(merged); - // [#13804] What "Save" means for the LIVE pool, decided by what actually // changed — never unconditionally. The old tail called `registerPool` on // every update, and the connect path's idempotency guard turned that into @@ -1023,12 +1051,38 @@ export class DatasourceAdminService implements IDatasourceAdminService { // cannot see: a rewrap-in-place keeps the `credentialsRef` string while // changing the credential it dereferences to, and the pool reads the // credential only at build time. + // + // [#29879] The rebuild runs BEFORE the record is stored, and a rebuild + // that keeps the previous pool stores nothing: not the record, not the + // new `credentialsRef`, not any other key of this patch. The answer says + // the new configuration is NOT in force, so the stored record has to be + // the one still serving, or the next restart connects the configuration + // that just failed — with the working credential already removed. One + // write path: the record is stored once, after the pool settles, and the + // working secret is retired only after that write. const rebuildNeeded = secret !== undefined || datasourceConnectivityChanged(existing, merged); + if (merged.active !== false && rebuildNeeded) { + const { previousPoolKept } = await this.tryReregisterPool(existing, merged); + if (previousPoolKept) { + await this.discardUnstoredSecret(name, newRef, prevRef); + // Nothing was stored, so there is nothing for a peer to converge on. + return this.toSummary(existing); + } + } + + try { + await this.config.putDatasourceRecord(merged); + } catch (err) { + // The stored record still names the working secret; the one sealed for + // this write has no referrer, so take it back out rather than orphan it. + if (newRef && newRef !== prevRef) await this.tryRemoveSecret(newRef); + throw err; + } + if (newRef && prevRef && prevRef !== newRef) await this.tryRemoveSecret(prevRef); + if (merged.active === false) { if (rebuildNeeded) await this.tryUnregisterPool(name); - } else if (rebuildNeeded) { - await this.tryReregisterPool(existing, merged); - } else { + } else if (!rebuildNeeded) { await this.tryRegisterPool(merged); } // [#13805] Unconditionally — a label-only edit included. The peer, not the @@ -1038,6 +1092,29 @@ export class DatasourceAdminService implements IDatasourceAdminService { return this.toSummary(merged); } + /** + * [#29879] Take back the secret an update sealed for a configuration that + * never came into force. A binder that mints a fresh reference per write + * (the shipped `sys_secret` binder) leaves the working row untouched, so + * removing the new one restores the stored binding exactly. A binder that + * rewrites in place under the SAME reference has already overwritten the + * working credential: that row is the binding the stored record still + * names, so it is never removed, and the loss is said once, at `error`. + */ + private async discardUnstoredSecret(name: string, newRef: string | undefined, prevRef: string | undefined): Promise { + if (!newRef) return; + if (newRef !== prevRef) { + await this.tryRemoveSecret(newRef); + return; + } + this.logger?.error?.( + `datasource '${name}': the new secret did not connect and the previous pool was kept, but the ` + + `secret store rewrote credential '${newRef}' in place, so the stored record now names the ` + + 'credential that failed and the datasource will not connect after a restart. Send the working ' + + 'credential again as `secret` to restore it.', + ); + } + /** * [#6504] Remove a runtime datasource, refusing when the bound-object count * behind the safety guard cannot be trusted. @@ -1523,15 +1600,20 @@ export class DatasourceAdminService implements IDatasourceAdminService { * re-registered would honour the rebuild but lose the "keep the old pool on * failure" half of the ruling. */ - private async tryReregisterPool(previous: StoredDatasource, next: StoredDatasource): Promise { + private async tryReregisterPool( + previous: StoredDatasource, + next: StoredDatasource, + ): Promise<{ previousPoolKept: boolean }> { if (!this.config.reregisterPool) { await this.tryRegisterPool(next); - return; + return { previousPoolKept: false }; } try { - await this.config.reregisterPool(previous, next); + const outcome = await this.config.reregisterPool(previous, next); + return { previousPoolKept: outcome?.previousPoolKept === true }; } catch (err) { this.logger?.warn(`reregisterPool('${next.name}') failed`, err); + return { previousPoolKept: false }; } } From 761e6ba3a9dcc8a829ac110ad35dbd9970a10256 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 20:52:37 +0000 Subject: [PATCH 2/4] test(service-datasource): pin the kept-pool update across a restart, and blank secrets at the door Claude-Session: https://claude.ai/code/session_01WGufzfDo764sexAMz4Feui Co-authored-by: Claude --- ...asource-update-kept-pool-stores-nothing.md | 18 + .../datasource-empty-secret-refusal.test.ts | 34 +- ...ce-update-kept-pool-stores-nothing.test.ts | 314 ++++++++++++++++++ 3 files changed, 358 insertions(+), 8 deletions(-) create mode 100644 .changeset/29879-datasource-update-kept-pool-stores-nothing.md create mode 100644 packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts diff --git a/.changeset/29879-datasource-update-kept-pool-stores-nothing.md b/.changeset/29879-datasource-update-kept-pool-stores-nothing.md new file mode 100644 index 00000000000..79bba6191d6 --- /dev/null +++ b/.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 + + + +**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. diff --git a/packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts b/packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts index 262ccac0433..1d3fa6db376 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-empty-secret-refusal.test.ts +++ b/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 { 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(); diff --git a/packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts b/packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts new file mode 100644 index 00000000000..5bd3a4cd6d2 --- /dev/null +++ b/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; + +/** 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(), seq: 0 }; +} +type Store = ReturnType; + +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): 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 { + 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 { + 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 { + 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(); + const data = { + findOne: async (table: string, q: { where: Record }) => { + assertEngineFindOnePredicate(table, q); + return rowsOf(store, table).find((r) => matches(r, q.where)) ?? null; + }, + find: async (table: string, q?: { where?: Record; 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 }) => { + 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 }) => { + 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>(); + 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([ + ['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?.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.status).toBe('error'); + expect(res.body.data.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.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.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.statusReason).toContain('the new configuration is NOT in force'); + } + expect(atRest(store)).toEqual(before); + expect((await boot(store)).factory.built).toEqual([REAL_PASSWORD]); + }); +}); From 472241f51c08bad4c89bcfca468a2168ff045e5f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 20:54:35 +0000 Subject: [PATCH 3/4] test(service-datasource): read the summary under data.datasource Claude-Session: https://claude.ai/code/session_01WGufzfDo764sexAMz4Feui Co-authored-by: Claude --- ...atasource-update-kept-pool-stores-nothing.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts b/packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts index 5bd3a4cd6d2..1a79eb429e1 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts +++ b/packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts @@ -236,7 +236,7 @@ function atRest(store: Store) { 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?.status }).toEqual({ status: 201, dsStatus: 'ok' }); + expect({ status: res.status, dsStatus: res.body.data?.datasource?.status }).toEqual({ status: 201, dsStatus: 'ok' }); return first; } @@ -253,8 +253,8 @@ describe('PATCH /api/v1/datasources/:name — a new secret that fails to connect label: 'Renamed in the same patch', }); expect(res.status).toBe(200); - expect(res.body.data.status).toBe('error'); - expect(res.body.data.statusReason).toContain('the new configuration is NOT in force'); + 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. @@ -275,7 +275,7 @@ describe('PATCH /api/v1/datasources/:name — a new secret that fails to connect const res = await first.send('PATCH', `/datasources/${CREATE.name}`, { config: { ...CREATE.config, database: 'qa_other' }, }); - expect(res.body.data.statusReason).toContain('the new configuration is NOT in force'); + 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]); }); @@ -287,7 +287,7 @@ describe('PATCH /api/v1/datasources/:name — a new secret that fails to connect const res = await first.send('PATCH', `/datasources/${CREATE.name}`, { secret: REAL_PASSWORD }); expect(res.status).toBe(200); - expect(res.body.data.status).toBe('ok'); + expect(res.body.data.datasource.status).toBe('ok'); const after = atRest(store); expect(after.record.external.credentialsRef).not.toBe(before.record.external.credentialsRef); @@ -306,7 +306,7 @@ describe('PATCH /api/v1/datasources/:name — a new secret that fails to connect 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.statusReason).toContain('the new configuration is NOT in force'); + 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]); From 2e46f1637f33ef14359ec783959fea38419b2016 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 21:30:11 +0000 Subject: [PATCH 4/4] test(service-datasource): pin the restart rig's engine doubles in the ledger Claude-Session: https://claude.ai/code/session_01WGufzfDo764sexAMz4Feui Co-authored-by: Claude --- scripts/engine-double-contract.pinned.json | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index afedff6ea52..566599e5eec 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -5231,6 +5231,21 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/services/service-datasource/src/__tests__/datasource-update-kept-pool-stores-nothing.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/services/service-job/src/db-job-adapter.degraded-outcome.test.ts", "verb": "update",