diff --git a/.changeset/datasource-update-rebuilds-pool.md b/.changeset/datasource-update-rebuilds-pool.md new file mode 100644 index 0000000000..3256be2f3c --- /dev/null +++ b/.changeset/datasource-update-rebuilds-pool.md @@ -0,0 +1,45 @@ +--- +"@objectstack/service-datasource": minor +--- + +Updating a datasource now rebuilds its live pool when the change actually bears on connectivity — and `active: false` actually takes it out of service + +`updateDatasource` persisted the merged record and called `registerPool`, whose +connect-path idempotency guard answered `already-registered` while the OLD +driver held the name and returned before building anything. Nothing on the +update path called `disconnect` first. So reconfiguring a datasource — new +host, new credentials, new pool settings, `active: false` — changed the stored +record and left the running connection untouched until process restart, while +`toSummary` kept reporting the ORIGINAL connect's retained `connected`: a +successful save describing a pool the record no longer declared. The +`active: false` corner is security-adjacent — an explicitly disabled data +plane kept serving. + +What "Save" now means for the live pool, decided by what actually changed: + +- **A connectivity-bearing field changed** (`driver`, `config`, `external` + including `credentialsRef`, `pool`, `schemaMode`, `active` — the set verified + against what `attemptConnect` reads into driver construction; a supplied + secret counts too, since a rewrap-in-place changes the credential without + changing the ref) → the pool is rebuilt in place via the new + `DatasourceConnectionService.reconnect`: the old registration is evicted + (the #13578 door), a new driver is built FROM THE NEW RECORD through the one + shared connect path, and the replaced pool's connection is closed. +- **The rebuild fails** → the OLD driver instance is restored (with the + datasource def eviction removed alongside it), so the datasource is never + left pool-less: the previous configuration keeps serving while the retained + verdict is loudly degraded and says exactly that. Runtime-admin writes still + never brick a running server over a UI action. +- **`active: false`** → the pool is torn down and the registry stops answering + the name — matching every other lifecycle door (`connectDeclared` and boot + rehydration never build a pool for a disabled record). `createDatasource` + gets the same guard: a datasource born disabled no longer comes up serving. +- **Nothing connectivity-bearing changed** (a label edit) → the idempotent + no-op path, exactly as before: same driver instance, no eviction, no + connection churn. + +Hosts wiring `DatasourceAdminServiceConfig` directly get the rebuild by +supplying the new optional `reregisterPool` seam; without it the behaviour is +unchanged (the idempotent register). The comparison itself is exported as +`datasourceConnectivityChanged` so a custom seam can ask the same question the +shipped update path asks. diff --git a/packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts b/packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts new file mode 100644 index 0000000000..33ef8a0efe --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts @@ -0,0 +1,483 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13804 — updating a datasource must rebuild its live pool exactly when a + * connectivity-bearing field changed, and only then. + * + * The defect: `updateDatasource` persisted the merged record and called + * `registerPool`, whose connect-path idempotency guard answered + * `already-registered` while the OLD driver held the name — so the running + * pool never followed the record, and `toSummary` kept reporting the ORIGINAL + * connect's retained `connected`, i.e. a successful save describing a pool the + * record no longer declares. `active: false` not taking effect is the + * security-adjacent corner of the same hole: an explicitly disabled data plane + * kept serving until process restart. + * + * The rig wires the REAL `DatasourceAdminService` + `DatasourceConnectionService` + * together exactly as `DatasourceAdminServicePlugin` does (registerPool → + * connect, unregisterPool → disconnect, reregisterPool → reconnect, + * connectionStates → listConnectionStates), against a fake engine that mirrors + * the real registry's semantics — `registerDriver` KEEPS the incumbent on a + * name collision, `unregisterDriver` removes driver + datasource def + default + * together — because those two behaviours are precisely why the update path + * needed an explicit rebuild primitive. + * + * "Serving" at this seam: the engine routes a query by consulting the driver + * registry FIRST (a registered driver answers before the unavailable mark is + * even read — see `ObjectQLEngine.getDriver`), so "the registry holds a live + * driver instance whose pool is open" is what serving means here, and "the + * registry no longer answers the name" is what stopping means. + */ + +import { describe, it, expect } from 'vitest'; +import { + DatasourceAdminService, + type DatasourceAdminServiceConfig, + type StoredDatasource, +} from '../datasource-admin-service.js'; +import { + DatasourceConnectionService, + type ConnectionEngineLike, +} from '../datasource-connection-service.js'; +import { datasourceConnectivityChanged } from '../datasource-connectivity-change.js'; +import type { + IDatasourceDriverFactory, + DatasourceConnectionSpec, +} from '../contracts/datasource-driver-factory.js'; +import type { DatasourceDraft } from '../contracts/index.js'; +import type { IDataDriver } from '@objectstack/spec/contracts'; + +/** The fake driver a pool is: it knows what it was built from and whether its connection is open. */ +interface FakeDriver { + name: string; + /** The exact `factory.create` input this pool was built from — the positive pin's subject. */ + builtFrom: DatasourceConnectionSpec & { secret?: string }; + connected: boolean; + closed: boolean; + disconnect: () => Promise; +} + +/** A fake engine mirroring the REAL registry semantics the fix depends on. */ +function fakeEngine() { + const drivers = new Map(); + const defs = new Map(); + const unavailable = new Map(); + const evicted: string[] = []; + const engine: ConnectionEngineLike & { + drivers: typeof drivers; + defs: typeof defs; + unavailable: typeof unavailable; + evicted: string[]; + } = { + drivers, + defs, + unavailable, + evicted, + registerDriver: (driver) => { + // Mirror the real engine: a name collision KEEPS the incumbent and + // discards the newcomer — the reason `connect()` alone cannot swap. + if (drivers.has(driver.name)) return; + drivers.set(driver.name, driver as unknown as FakeDriver); + }, + unregisterDriver: (name) => { + // Mirror the real engine: the def is removed together with the driver + // (the registry owns that invariant) — the reason `reconnect` must + // restore the def on the keep-old-pool path. + defs.delete(name); + evicted.push(name); + return drivers.delete(name); + }, + getDriverByName: (name) => drivers.get(name) as unknown as IDataDriver | undefined, + registerDatasourceDef: (def) => { + defs.set(def.name, def); + }, + markDatasourceUnavailable: (info) => { + unavailable.set(info.name, info); + }, + clearDatasourceUnavailable: (name) => { + unavailable.delete(name); + }, + }; + return engine; +} + +/** A fake factory whose drivers capture the spec they were built from. */ +function fakeFactory(opts: { failWhen?: (spec: DatasourceConnectionSpec & { secret?: string }) => boolean } = {}) { + const created: Array = []; + const factory: IDatasourceDriverFactory & { created: typeof created } = { + created, + supports: () => true, + create: async (spec) => { + created.push(spec as DatasourceConnectionSpec & { secret?: string }); + const driver: FakeDriver = { + name: 'com.fake.driver', + builtFrom: spec as DatasourceConnectionSpec & { secret?: string }, + connected: false, + closed: false, + disconnect: async () => { + driver.closed = true; + }, + }; + return { + driver, + connect: async () => { + if (opts.failWhen?.(driver.builtFrom)) throw new Error('connection refused'); + driver.connected = true; + }, + }; + }, + }; + return factory; +} + +/** + * The plugin's wiring, reproduced: one admin service and one connection + * service sharing an engine, a factory, and a secret store. + */ +function makeRig(opts: { + failWhen?: (spec: DatasourceConnectionSpec & { secret?: string }) => boolean; + /** Rewrap-in-place binder: every write lands under ONE stable ref. */ + stableSecretRef?: boolean; + /** Leave the `reregisterPool` seam unwired (a pre-#13804 host). */ + noReregisterSeam?: boolean; +} = {}) { + const engine = fakeEngine(); + const factory = fakeFactory({ failWhen: opts.failWhen }); + const records: StoredDatasource[] = []; + const secrets = new Map(); + let secretSeq = 0; + + const connection = new DatasourceConnectionService({ + factory: () => factory, + engine: () => engine, + secrets: { resolve: async (ref) => secrets.get(ref) }, + }); + + const config: DatasourceAdminServiceConfig = { + probe: async () => ({ ok: true }), + listDatasourceRecords: async () => records.map((r) => ({ ...r })), + getDatasourceRecord: async (n) => { + const r = records.find((x) => x.name === n); + return r ? { ...r } : undefined; + }, + putDatasourceRecord: async (record) => { + const idx = records.findIndex((r) => r.name === record.name); + if (idx >= 0) records[idx] = { ...record }; + else records.push({ ...record }); + }, + deleteDatasourceRecord: async (n) => { + const idx = records.findIndex((r) => r.name === n); + if (idx >= 0) records.splice(idx, 1); + }, + writeSecret: async (input, hint) => { + const ref = opts.stableSecretRef + ? `sys_secret://datasource/${hint.name}` + : `sys_secret://datasource/${hint.name}#${++secretSeq}`; + secrets.set(ref, input.value); + return ref; + }, + countBoundObjects: async () => 0, + registerPool: (record) => + connection.connect(record, { + context: { origin: record.origin ?? 'runtime', trigger: 'runtime-admin' }, + }).then(() => undefined), + unregisterPool: (name) => connection.disconnect(name), + ...(opts.noReregisterSeam + ? {} + : { + reregisterPool: (previous: StoredDatasource, next: StoredDatasource) => + connection + .reconnect(next, { + previous, + context: { origin: next.origin ?? 'runtime', trigger: 'runtime-admin' }, + }) + .then(() => undefined), + }), + connectionStates: () => connection.listConnectionStates(), + }; + + const service = new DatasourceAdminService(config); + return { service, connection, engine, factory, secrets, records }; +} + +const draft = (over: Partial = {}): DatasourceDraft => ({ + name: 'analytics', + driver: 'sqlite', + config: { filename: '/tmp/old.db' }, + ...over, +}); + +describe('#13804 — updateDatasource rebuilds the live pool when connectivity changed', () => { + it('a config change evicts the old pool and registers a NEW pool genuinely built from the NEW config', async () => { + const rig = makeRig(); + await rig.service.createDatasource(draft()); + const oldDriver = rig.engine.drivers.get('analytics')!; + expect(oldDriver.builtFrom.config).toMatchObject({ filename: '/tmp/old.db' }); + expect(oldDriver.connected).toBe(true); + + const summary = await rig.service.updateDatasource('analytics', { + config: { filename: '/tmp/new.db' }, + }); + + const newDriver = rig.engine.drivers.get('analytics')!; + // The pin is on WHAT the registered pool was built from — not on any + // eviction call having happened (that is an implementation detail). + expect(newDriver).not.toBe(oldDriver); + expect(newDriver.builtFrom.config).toMatchObject({ filename: '/tmp/new.db' }); + expect(newDriver.connected).toBe(true); + // The replaced pool's connection is closed, not leaked. + expect(oldDriver.closed).toBe(true); + // The save's verdict describes the pool that now exists. + expect(summary.status).toBe('ok'); + }); + + it('active: false takes the datasource out of service — the registry no longer answers the name', async () => { + const rig = makeRig(); + await rig.service.createDatasource(draft()); + const oldDriver = rig.engine.drivers.get('analytics')!; + + const summary = await rig.service.updateDatasource('analytics', { active: false }); + + // The engine routes by consulting the driver registry first, so an entry + // that is gone is a datasource that no longer serves — the security- + // adjacent half of the card: disabling must actually disable. + expect(rig.engine.drivers.has('analytics')).toBe(false); + expect(oldDriver.closed).toBe(true); + expect(summary.active).toBe(false); + // Verdict matches the real pool state: nothing is connected and nothing + // was attempted — the same reading a boot gives a disabled datasource + // (never connected, no retained verdict), not a stale `ok`. + expect(summary.status).toBe('unvalidated'); + expect(rig.connection.getConnectionState('analytics')).toBeUndefined(); + }); + + it('active: true re-enables — the pool is rebuilt from the stored record', async () => { + const rig = makeRig(); + await rig.service.createDatasource(draft()); + await rig.service.updateDatasource('analytics', { active: false }); + expect(rig.engine.drivers.has('analytics')).toBe(false); + + const summary = await rig.service.updateDatasource('analytics', { active: true }); + + const driver = rig.engine.drivers.get('analytics')!; + expect(driver.connected).toBe(true); + expect(driver.builtFrom.config).toMatchObject({ filename: '/tmp/old.db' }); + expect(summary.status).toBe('ok'); + }); + + it('reverse control: a label-only edit is the SAME driver instance — no eviction, no rebuild, no churn', async () => { + const rig = makeRig(); + await rig.service.createDatasource(draft()); + const oldDriver = rig.engine.drivers.get('analytics')!; + expect(rig.factory.created).toHaveLength(1); + + const summary = await rig.service.updateDatasource('analytics', { label: 'Renamed' }); + + // Identity, not equivalence: the rejected always-swap design would pass an + // equivalence check by rebuilding an identical pool. Only instance + // identity distinguishes "left alone" from "churned". + expect(rig.engine.drivers.get('analytics')).toBe(oldDriver); + expect(rig.factory.created).toHaveLength(1); + expect(rig.engine.evicted).toHaveLength(0); + expect(oldDriver.closed).toBe(false); + expect(summary.label).toBe('Renamed'); + expect(summary.status).toBe('ok'); + }); + + it('reverse control: round-tripping an unchanged config + external is not a change', async () => { + const rig = makeRig(); + await rig.service.createDatasource( + draft({ schemaMode: 'external', external: { allowWrites: false } }), + ); + const oldDriver = rig.engine.drivers.get('analytics')!; + + // The wizard PATCHes the full document back: same config values (a new + // object), same external block. The merge writes `credentialsRef: + // undefined` onto `external` — a key the stored record never had — and the + // comparison must read that as "no change" (deep equality + JSON's + // undefined-is-absent), not rebuild the pool on every save. + await rig.service.updateDatasource('analytics', { + config: { filename: '/tmp/old.db' }, + external: { allowWrites: false }, + }); + + expect(rig.engine.drivers.get('analytics')).toBe(oldDriver); + expect(rig.factory.created).toHaveLength(1); + expect(rig.engine.evicted).toHaveLength(0); + }); + + it('rebuild failure keeps the OLD pool live and serving under a loudly degraded verdict — never pool-less', async () => { + const rig = makeRig({ + failWhen: (spec) => (spec.config as { filename?: string }).filename === '/tmp/bad.db', + }); + await rig.service.createDatasource( + draft({ schemaMode: 'external', external: { allowWrites: false } }), + ); + const oldDriver = rig.engine.drivers.get('analytics')!; + + const summary = await rig.service.updateDatasource('analytics', { + config: { filename: '/tmp/bad.db' }, + }); + + // 1. The old pool is still live and serving: same instance in the + // registry (routing consults the registry before the unavailable + // mark), connection still open. + expect(rig.engine.drivers.get('analytics')).toBe(oldDriver); + expect(oldDriver.closed).toBe(false); + expect(oldDriver.connected).toBe(true); + // 2. The verdict is loudly degraded — not `connected`, not `ok` — and + // says the truth: the previous configuration's pool is the one serving. + expect(summary.status).toBe('error'); + expect(summary.statusReason).toContain('connection refused'); + expect(summary.statusReason).toContain('still the one serving'); + const state = rig.connection.getConnectionState('analytics')!; + expect(state.status).toBe('failed-degraded'); + // 3. Not pool-less: the datasource def the old pool was serving under is + // restored alongside the driver (`unregisterDriver` removes both). + expect(rig.engine.defs.get('analytics')).toMatchObject({ + name: 'analytics', + schemaMode: 'external', + }); + }); + + it('a supplied secret alone forces a rebuild — rewrap-in-place changes what the ref dereferences to', async () => { + const rig = makeRig({ stableSecretRef: true }); + await rig.service.createDatasource( + draft({ schemaMode: 'external', external: { allowWrites: false } }), + { value: 'old-pw' }, + ); + const oldDriver = rig.engine.drivers.get('analytics')!; + expect(oldDriver.builtFrom.secret).toBe('old-pw'); + + // The record diff cannot see this change: the binder rewraps under the + // SAME `credentialsRef`, so `external` compares equal — yet the pool reads + // the credential only at build time, so without a rebuild the old + // password keeps being used. + await rig.service.updateDatasource('analytics', {}, { value: 'new-pw' }); + + const newDriver = rig.engine.drivers.get('analytics')!; + expect(newDriver).not.toBe(oldDriver); + expect(newDriver.builtFrom.secret).toBe('new-pw'); + expect(oldDriver.closed).toBe(true); + }); + + it('createDatasource honours active: false — no pool for a datasource born disabled', async () => { + const rig = makeRig(); + const summary = await rig.service.createDatasource(draft({ active: false })); + + // Same reading as every other lifecycle door: `connectDeclared` skips + // `active === false` at boot and rehydration filters on `active ?? true`. + // Create was the outlier that built a live pool for a disabled record. + expect(rig.factory.created).toHaveLength(0); + expect(rig.engine.drivers.size).toBe(0); + expect(summary.active).toBe(false); + expect(summary.status).toBe('unvalidated'); + }); + + it('a host without the reregisterPool seam degrades to the pre-#13804 idempotent register (old pool retained)', async () => { + const rig = makeRig({ noReregisterSeam: true }); + await rig.service.createDatasource(draft()); + const oldDriver = rig.engine.drivers.get('analytics')!; + + await rig.service.updateDatasource('analytics', { config: { filename: '/tmp/new.db' } }); + + // Not the fixed behaviour — the safe fallback: the incumbent pool stays + // (never pool-less, never a failed swap), exactly what this host had + // before the seam existed. + expect(rig.engine.drivers.get('analytics')).toBe(oldDriver); + expect(rig.factory.created).toHaveLength(1); + }); +}); + +describe('datasourceConnectivityChanged — the ruled field set, read from what attemptConnect consumes', () => { + const base: Pick = { + driver: 'postgres', + config: { host: 'db.internal', port: 5432 }, + external: { allowWrites: false, credentialsRef: 'sys_secret://x' }, + pool: { max: 10 }, + active: true, + }; + + it('each ruled field trips it', () => { + expect(datasourceConnectivityChanged(base, { ...base, driver: 'mysql' })).toBe(true); + expect( + datasourceConnectivityChanged(base, { ...base, config: { host: 'db.other', port: 5432 } }), + ).toBe(true); + expect( + datasourceConnectivityChanged(base, { + ...base, + external: { allowWrites: false, credentialsRef: 'sys_secret://y' }, + }), + ).toBe(true); + expect(datasourceConnectivityChanged(base, { ...base, pool: { max: 20 } })).toBe(true); + expect(datasourceConnectivityChanged(base, { ...base, active: false })).toBe(true); + }); + + it('a schemaMode-only edit trips it — the seventh member, ruled in by the contract review', () => { + // Three real readings on the connect path: the policy gate (`canConnect`), + // `toSpec` -> `factory.create` (the driver is built from it), and + // `registerDatasourceDef` (the write gate's def). It is patchable by + // `updateDatasource`, so without this member a schemaMode-only save + // persisted the new record while all three kept the OLD value until + // restart — a narrower instance of the stale-pool defect this card fixes. + expect(datasourceConnectivityChanged(base, { ...base, schemaMode: 'external' })).toBe(true); + // Both directions: first-time set, and cleared. + expect( + datasourceConnectivityChanged( + { driver: 'sqlite' }, + { driver: 'sqlite', schemaMode: 'validate-only' }, + ), + ).toBe(true); + expect( + datasourceConnectivityChanged( + { driver: 'sqlite', schemaMode: 'external' }, + { driver: 'sqlite' }, + ), + ).toBe(true); + // Widened by exactly ONE member, not into "rebuild on everything": an + // unchanged schemaMode is still no change, which is what leaves the + // label-only reverse control above reading the same as before. + expect( + datasourceConnectivityChanged( + { ...base, schemaMode: 'external' }, + { ...base, schemaMode: 'external' }, + ), + ).toBe(false); + }); + + it('deep-equal values are no change, whatever the object identity', () => { + const same = { + driver: 'postgres', + config: { host: 'db.internal', port: 5432 }, + external: { allowWrites: false, credentialsRef: 'sys_secret://x' }, + pool: { max: 10 }, + active: true, + }; + expect(datasourceConnectivityChanged(base, same)).toBe(false); + }); + + it('normalises the way the connect path reads: config ?? {}, active ?? true, undefined keys absent', () => { + // `toSpec` sends `config ?? {}`. + expect( + datasourceConnectivityChanged( + { driver: 'sqlite', config: undefined }, + { driver: 'sqlite', config: {} }, + ), + ).toBe(false); + // Spec default: `active` omitted means enabled. + expect( + datasourceConnectivityChanged( + { driver: 'sqlite', active: undefined }, + { driver: 'sqlite', active: true }, + ), + ).toBe(false); + // The update merge writes `credentialsRef: undefined` onto a record that + // never had the key — JSON semantics: not a change. + expect( + datasourceConnectivityChanged( + { driver: 'sqlite', external: { allowWrites: true } }, + { driver: 'sqlite', external: { allowWrites: true, credentialsRef: undefined } }, + ), + ).toBe(false); + }); +}); diff --git a/packages/services/service-datasource/src/datasource-admin-plugin.ts b/packages/services/service-datasource/src/datasource-admin-plugin.ts index ba9146353c..2ad1c57545 100644 --- a/packages/services/service-datasource/src/datasource-admin-plugin.ts +++ b/packages/services/service-datasource/src/datasource-admin-plugin.ts @@ -412,6 +412,19 @@ export class DatasourceAdminServicePlugin implements Plugin { await this.connection?.disconnect(name); }, + // [#13804] The update path's in-place rebuild: evict the pool built from + // the OLD record, rebuild from the NEW one through the same shared + // connect path, and on failure restore the old pool (never pool-less). + // `previous` rides along because the engine's eviction door removes the + // datasource def with the driver, and the keep-old-pool path must put + // back the def the old pool was serving under. + reregisterPool: async (previous, next) => { + await this.connection?.reconnect(next, { + previous, + context: { origin: next.origin ?? 'runtime', trigger: 'runtime-admin' }, + }); + }, + // The admin list's `status` reads the connection service's retained // verdicts (framework#3827). Resolved lazily per call: the service exists // by the end of this init(), but the verdicts only appear once boot diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index 9e65dff836..ba4305a958 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/packages/services/service-datasource/src/datasource-admin-service.ts @@ -32,6 +32,7 @@ import { validateDriverConfig } from '@objectstack/spec/data'; import { assertDatasourcePoolSupported } from './datasource-pool-support.js'; +import { datasourceConnectivityChanged } from './datasource-connectivity-change.js'; import { redactDatasourceConfig, restoreRedactedConfig } from './datasource-config-redaction.js'; import { planCredentialMigration } from './datasource-credential-migration.js'; import type { @@ -139,6 +140,17 @@ export interface DatasourceAdminServiceConfig { registerPool?: (record: StoredDatasource) => Promise | void; /** Tear down a runtime datasource's pool on remove. */ unregisterPool?: (name: string) => Promise | void; + /** + * Rebuild a runtime datasource's pool after an update that changed its + * connectivity-bearing fields (#13804): evict the pool built from + * `previous`, build + register a new one from `next`, and on failure KEEP + * the previous pool live with a loud degraded verdict (never pool-less). + * + * 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. + */ + reregisterPool?: (previous: StoredDatasource, next: StoredDatasource) => Promise | void; /** * Last connect verdict per datasource, from `DatasourceConnectionService` * (framework#3827). Absent (a host without the connection service) means the @@ -378,7 +390,12 @@ export class DatasourceAdminService implements IDatasourceAdminService { } await this.config.putDatasourceRecord(record); - await this.tryRegisterPool(record); + // [#13804] A record born `active: false` gets no live pool. Every other + // lifecycle path already reads the flag this way — `connectDeclared` skips + // `active === false` at boot, and boot rehydration filters on + // `active ?? true` — so this door was the one place a deliberately + // disabled datasource still came up serving. + if (record.active !== false) await this.tryRegisterPool(record); return this.toSummary(record); } @@ -464,7 +481,41 @@ export class DatasourceAdminService implements IDatasourceAdminService { } await this.config.putDatasourceRecord(merged); - await this.tryRegisterPool(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 + // `already-registered` while a driver held the name: the merged record was + // persisted, the pool built from the OLD record kept serving, and + // `toSummary` reported the ORIGINAL connect's retained `connected` — a + // successful save describing a pool the record no longer declares. + // + // Ruled decision tree (triage on the card): + // - a connectivity-bearing field changed (`driver` / `config` / + // `external` incl. `credentialsRef` / `pool` / `schemaMode` / `active` + // — the reading behind the set lives on + // `datasourceConnectivityChanged`) → rebuild + // in place, keeping the old pool if the rebuild fails; + // - `active: false` → tear the pool down: an explicitly disabled + // datasource must stop serving (matching boot, where it is never + // connected at all); + // - nothing connectivity-bearing changed (a label edit) → the plain + // idempotent register, exactly as before: no eviction, no rebuild, no + // connection churn — and its retry-a-broken-pool side effect (a no-op + // edit re-attempting a pool that failed at boot) is preserved. + // + // A supplied `secret` counts as a connectivity change the record diff + // 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. + const rebuildNeeded = secret !== undefined || datasourceConnectivityChanged(existing, merged); + if (merged.active === false) { + if (rebuildNeeded) await this.tryUnregisterPool(name); + } else if (rebuildNeeded) { + await this.tryReregisterPool(existing, merged); + } else { + await this.tryRegisterPool(merged); + } return this.toSummary(merged); } @@ -768,6 +819,26 @@ export class DatasourceAdminService implements IDatasourceAdminService { } } + /** + * [#13804] The in-place rebuild, degrade-not-throw like its siblings. A host + * without the `reregisterPool` seam falls back to the plain register: its + * idempotency guard keeps the incumbent pool, which is the pre-#13804 + * behaviour and the safe direction — a fallback that instead tore down and + * 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 { + if (!this.config.reregisterPool) { + await this.tryRegisterPool(next); + return; + } + try { + await this.config.reregisterPool(previous, next); + } catch (err) { + this.logger?.warn(`reregisterPool('${next.name}') failed`, err); + } + } + private async tryUnregisterPool(name: string): Promise { try { await this.config.unregisterPool?.(name); diff --git a/packages/services/service-datasource/src/datasource-connection-service.ts b/packages/services/service-datasource/src/datasource-connection-service.ts index 116bba6591..c9590bb83a 100644 --- a/packages/services/service-datasource/src/datasource-connection-service.ts +++ b/packages/services/service-datasource/src/datasource-connection-service.ts @@ -734,6 +734,124 @@ export class DatasourceConnectionService { if (driverName) engine?.unregisterDriver?.(driverName); } + /** + * Rebuild one datasource's live pool from a NEW record, keeping the OLD pool + * when the rebuild fails (#13804). + * + * The update path's problem is that {@link connect} alone cannot express a + * reconfigure: its idempotency guard answers `already-registered` while the + * old driver holds the name, and the engine's `registerDriver` keeps the + * incumbent on a collision — so a changed record was persisted while the + * pool built from the OLD record kept serving, with the retained `connected` + * verdict describing a configuration the record no longer declares. + * + * Sequence: evict the old registration (the #13578 door), rebuild via the + * one shared connect path, then settle by outcome — + * + * - **rebuild succeeded** → the new pool is registered; the old pool's + * connection is closed (unless the instance was ADOPTED — host-owned, + * ADR-0062 D5 — in which case the host keeps it, exactly as + * {@link disconnect} would). + * - **rebuild failed or was refused** → the OLD driver instance is + * re-registered so the datasource is never left pool-less (the ruled + * "never brick a running server over a UI action" direction), and the + * retained verdict stays the LOUD failed one — the admin list reports + * `error`/`blocked` with a reason naming that the previous + * configuration's pool is still the one serving. Routing consults the + * registry before the unavailable mark, so the restored driver keeps + * serving while the verdict tells the operator the truth. + * + * The eviction window is real but narrow: between the evict and the new + * registration, queries against this datasource fail with "not registered". + * That is the cost of an in-place rebuild; the caller only takes it when a + * connectivity-bearing field actually changed (a label edit never enters). + * + * `previous` is the record the LIVE pool was built from. It exists because + * `unregisterDriver` removes the datasource def together with the driver + * (the registry owns that invariant), so the keep-old-pool path must restore + * the def the old pool was serving under — otherwise the write gate would + * judge writes for a datasource whose def vanished mid-failure. + * + * Not for the DEFAULT datasource: runtime-admin records can never be named + * `default` (the name is host-reserved), and the default driver's natural- + * name registration (#3826) would make a name-keyed swap evict nothing. + */ + async reconnect( + record: ConnectableDatasource, + opts: { + context?: DatasourceConnectContext; + previous?: Pick; + } = {}, + ): Promise { + const name = record.name; + const engine = this.cfg.engine(); + const oldDriver = engine?.getDriverByName?.(name); + if (!oldDriver || !engine || typeof engine.unregisterDriver !== 'function') { + // Nothing to swap (no live driver — the datasource was inactive, or its + // last connect failed), or the engine has no eviction door. Either way a + // plain connect is the whole job: it builds when the name is free, and + // on a door-less engine it degrades to the pre-#13804 idempotent no-op — + // the safe direction for a host that cannot express a swap. + return this.connect(record, { context: opts.context }); + } + const oldOwnership = this.states.get(name)?.ownership; + + const restoreOldPool = (): void => { + engine.registerDriver?.(oldDriver, false); + engine.registerDatasourceDef?.({ + name, + schemaMode: opts.previous?.schemaMode, + external: opts.previous?.external as { allowWrites?: boolean } | undefined, + }); + }; + const stillServingSuffix = + " — the previous configuration's pool was kept and is still the one serving; the new configuration is NOT in force"; + + engine.unregisterDriver(name); + let result: ConnectResult; + try { + result = await this.connect(record, { context: opts.context }); + } catch (err) { + // `connect` re-throws authoring verdicts (an unhonourable `pool` block, + // #5714) after recording a failed state. Same settlement as a returned + // failure: the old pool comes back, the loud verdict stands. + restoreOldPool(); + this.amendRetainedReason(name, stillServingSuffix); + throw err; + } + + if (availabilityOf(result.status) === 'available') { + if (oldOwnership !== 'host' && typeof oldDriver.disconnect === 'function') { + try { + await oldDriver.disconnect(); + } catch (err) { + this.logger?.warn?.(`datasource '${name}': closing the replaced pool failed: ${errMsg(err)}`); + } + } + return result; + } + + restoreOldPool(); + this.amendRetainedReason(name, stillServingSuffix); + this.logger?.warn?.( + `datasource '${name}': rebuild after reconfigure failed (${result.reason ?? result.status})` + + stillServingSuffix, + ); + return result; + } + + /** + * Append detail to the retained verdict's `reason` — used by + * {@link reconnect} so the loud degraded verdict also says the OLD pool is + * the one still serving (without it, `error` + a bare connect failure reads + * as "this datasource is down", which is precisely not the state). + */ + private amendRetainedReason(name: string, suffix: string): void { + const st = this.states.get(name); + if (!st) return; + this.states.set(name, { ...st, reason: `${st.reason ?? 'reconnect failed'}${suffix}` }); + } + /** * Kernel-teardown sweep (ADR-0062 D5, #3993): disconnect exactly the pools * THIS service opened — states with status `'connected'`, nothing else. diff --git a/packages/services/service-datasource/src/datasource-connectivity-change.ts b/packages/services/service-datasource/src/datasource-connectivity-change.ts new file mode 100644 index 0000000000..a9044f2c7f --- /dev/null +++ b/packages/services/service-datasource/src/datasource-connectivity-change.ts @@ -0,0 +1,120 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Which fields of a stored datasource record bear on CONNECTIVITY — i.e. on + * what `DatasourceConnectionService.attemptConnect` builds the live pool from + * (#13804). + * + * ## Why this set, read from the tree rather than from the record + * + * The update path rebuilds a pool only when one of these changed. The set was + * verified by working BACKWARDS from what `attemptConnect` actually reads into + * the driver construction, not forwards from the record's field list: + * + * - `driver` — read by the pool-support gate, the connect policy, and + * `toSpec` (the `factory.create` input). + * - `config` — read wholesale by `toSpec` (`config: record.config ?? {}`). + * No sub-key is excluded on that path: the entire block is an input to + * `factory.create`, so the whole block is compared here. + * - `external` — read by the connect policy, `toSpec`, and + * `registerDatasourceDef`; its `credentialsRef` sub-key drives the + * fail-closed secret resolution (ADR-0062 D3). Comparing the block deep + * therefore covers the ruled `credentialsRef` member too. A supplied + * cleartext secret is the one credential change this comparison CANNOT see + * (a rewrap-in-place keeps the ref string while changing what it + * dereferences to), which is why `updateDatasource` treats "a secret was + * supplied" as a connectivity change alongside this function's verdict. + * - `pool` — read by the pool-support gate and `toSpec`. + * - `schemaMode` — read by the connect policy gate (`canConnect`), by + * `toSpec` (so it reaches `factory.create` and the driver it builds), and + * by `registerDatasourceDef` (the write gate's def). It is patchable on + * the update path, so all three of those readings can go stale. + * - `active` — never read by `attemptConnect`, but it governs whether a pool + * may exist at all: `connectDeclared` skips `active === false` at boot and + * rehydration filters on `active ?? true`, so an update flipping it must + * tear down or build accordingly. + * + * `label` is read by nothing on the connect path — the reverse control: an + * edit to it must not churn a working connection. + * + * How `schemaMode` joined the set — RESOLVED, not open. It was found during + * this card's premise verification and reported as a fork rather than added + * unilaterally (an implementer does not widen a ruled set on its own). The + * contract review then ruled it IN, in the same stroke: it is really read at + * the three sites listed above, so leaving it out would have left a narrower + * instance of the very stale-pool defect this module exists to close — a + * schemaMode-only edit persisting a new record while the engine's datasource + * def, the driver, and the policy decision all kept the OLD value until + * restart. + * + * Two candidates were examined and are deliberately NOT members: `ssl` is a + * `toSpec` input but is not a field of `StoredDatasource` or + * `DatasourceDraft`, so it cannot change through the update path at all; and + * `autoConnect` is neither patchable by `updateDatasource` nor read by + * `attemptConnect`. + */ + +import type { StoredDatasource } from './datasource-admin-service.js'; + +/** The slice of a stored record this comparison consults. */ +export type ConnectivityBearingFields = Pick< + StoredDatasource, + 'driver' | 'config' | 'external' | 'pool' | 'schemaMode' | 'active' +>; + +/** + * Did an update change what the live pool was built from? + * + * Normalisations mirror the connect path, not JavaScript identity: + * - `config` compares `?? {}` because `toSpec` sends `record.config ?? {}`. + * - `active` compares `?? true` because that is the spec default and the + * boot-rehydration filter's reading. + * - Keys holding `undefined` count as absent (JSON semantics, and the merge + * in `updateDatasource` writes `credentialsRef: undefined` onto a record + * that never had the key — that round-trip is not a change). + * - `schemaMode` compares strictly, with no default applied: the connect + * path applies none either (`toSpec` omits the key when the record has no + * value; the policy gate and `registerDatasourceDef` receive it raw), so + * an absent value and a defaulted one are not the same reading here. + */ +export function datasourceConnectivityChanged( + before: ConnectivityBearingFields, + after: ConnectivityBearingFields, +): boolean { + if (before.driver !== after.driver) return true; + if (!deepEqual(before.config ?? {}, after.config ?? {})) return true; + if (!deepEqual(before.external, after.external)) return true; + if (!deepEqual(before.pool, after.pool)) return true; + if (before.schemaMode !== after.schemaMode) return true; + if ((before.active ?? true) !== (after.active ?? true)) return true; + return false; +} + +/** Keys whose value is not `undefined` — the JSON reading of "present". */ +function presentKeys(obj: Record): string[] { + return Object.keys(obj).filter((k) => obj[k] !== undefined); +} + +/** + * Deep equality over JSON-shaped data (plain objects, arrays, primitives) — + * the only shapes a persisted datasource record can hold, since every row + * round-trips through `JSON.stringify` in the sys_metadata store. + */ +function deepEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a === null || b === null) return false; + if (typeof a !== 'object' || typeof b !== 'object') return false; + const aArr = Array.isArray(a); + const bArr = Array.isArray(b); + if (aArr !== bArr) return false; + if (aArr && bArr) { + if (a.length !== b.length) return false; + return a.every((v, i) => deepEqual(v, b[i])); + } + const ao = a as Record; + const bo = b as Record; + const ak = presentKeys(ao); + const bk = presentKeys(bo); + if (ak.length !== bk.length) return false; + return ak.every((k) => deepEqual(ao[k], bo[k])); +} diff --git a/packages/services/service-datasource/src/index.ts b/packages/services/service-datasource/src/index.ts index 0e6b1319af..00857c8a9d 100644 --- a/packages/services/service-datasource/src/index.ts +++ b/packages/services/service-datasource/src/index.ts @@ -75,6 +75,12 @@ export type { ProbeInput, } from './datasource-admin-service.js'; +// Which update actually changes what the live pool was built from (#13804) — +// exported so a host wiring its own `reregisterPool` seam asks the same +// question the shipped update path asks, rather than re-deriving the set. +export { datasourceConnectivityChanged } from './datasource-connectivity-change.js'; +export type { ConnectivityBearingFields } from './datasource-connectivity-change.js'; + // Kernel plugin (registers the `'datasource-admin'` service). export { DatasourceAdminServicePlugin } from './datasource-admin-plugin.js'; export type {