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..1a79eb429e1 --- /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?.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]); + }); +}); 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 }; } } 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",