From 055d694b3e116d5fff79b467104c35a7d04dab12 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 11:18:28 +0000 Subject: [PATCH 1/3] fix(service-datasource): rebuild the live pool when an update changes connectivity-bearing fields (#13804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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'. An explicitly disabled datasource (active: false) kept serving until restart. Ruled decision tree: rebuild only when driver/config/external(credentialsRef)/ pool/active actually changed; active: false tears the pool down; on rebuild failure the OLD pool is kept live under a loudly degraded verdict (never pool-less). Label-only edits keep the idempotent no-op path: same driver instance, no churn. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .../datasource-update-rebuild.test.ts | 451 ++++++++++++++++++ .../src/datasource-admin-plugin.ts | 13 + .../src/datasource-admin-service.ts | 74 ++- .../src/datasource-connection-service.ts | 118 +++++ .../src/datasource-connectivity-change.ts | 104 ++++ .../services/service-datasource/src/index.ts | 6 + 6 files changed, 764 insertions(+), 2 deletions(-) create mode 100644 packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts create mode 100644 packages/services/service-datasource/src/datasource-connectivity-change.ts 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..a8e26d6110 --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts @@ -0,0 +1,451 @@ +// 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('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..79bc33e7ee 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,40 @@ 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` / `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 +818,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..ac831257bd --- /dev/null +++ b/packages/services/service-datasource/src/datasource-connectivity-change.ts @@ -0,0 +1,104 @@ +// 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`. + * - `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. + * + * ⚠️ Known fork, reported on the card rather than resolved here: `schemaMode` + * is patchable on the update path AND is read by `attemptConnect` (policy + * gate, `toSpec` → `factory.create`, `registerDatasourceDef`), yet the ruled + * set omits it — so a schemaMode-only edit does not rebuild, and the engine's + * datasource def keeps the OLD schemaMode until restart. Deliberately not + * added: the ruling fixed the set, and widening it is the maintainer's call. + * (`ssl` is also a `toSpec` input but is not a field of `StoredDatasource` or + * `DatasourceDraft`, so it cannot change through this path.) + */ + +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' | '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). + */ +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.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 { From dc7c99a2b2f4cdecaed4ba2244a6d4a3eefdab80 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 11:21:05 +0000 Subject: [PATCH 2/3] chore: add changeset for #13804 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .changeset/datasource-update-rebuilds-pool.md | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 .changeset/datasource-update-rebuilds-pool.md diff --git a/.changeset/datasource-update-rebuilds-pool.md b/.changeset/datasource-update-rebuilds-pool.md new file mode 100644 index 0000000000..5b95e7e510 --- /dev/null +++ b/.changeset/datasource-update-rebuilds-pool.md @@ -0,0 +1,45 @@ +--- +"@objectstack/service-datasource": patch +--- + +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`, `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. From 84cb0b6e6174b244cf33b901f74ef404b88f26cf Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 14:49:10 +0000 Subject: [PATCH 3/3] fix(service-datasource): rule schemaMode into the connectivity-bearing set (#13804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract-review increment on PR #14196 (director-seat conditional PASS, comment 5494985273). Two changes, exactly as ruled: 1. The changeset for @objectstack/service-datasource moves patch -> minor. The diff adds public API — the `datasourceConnectivityChanged` and `ConnectivityBearingFields` exports, the public `DatasourceConnectionService.reconnect` method, and the published `DatasourceAdminServiceConfig.reregisterPool` member — which is additive widening = minor by repo convention (#13897 is the same shape). `patch` under-reported the surface movement. 2. `schemaMode` joins the connectivity-bearing field set. It was found during this card's premise verification and reported as a fork rather than added unilaterally; the review ruled it IN in the same stroke. It is really read at three sites on the connect path — the `canConnect` policy gate, `toSpec` -> `factory.create` (driver construction), and `registerDatasourceDef` (the write gate's def) — and it is patchable by `updateDatasource`, so omitting it left a schemaMode-only save persisting the new record while all three kept the OLD value until restart: a narrower instance of the stale-pool defect this card fixes. One comparator line, plus the exported field slice, plus one comparator pin. The module docblock now states the resolution instead of carrying the fork as an open question. The label-only reverse control (same driver instance, zero factory calls, zero evictions) stays green, which is what shows the set widened by exactly one member rather than into "rebuild on everything". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .changeset/datasource-update-rebuilds-pool.md | 10 +++--- .../datasource-update-rebuild.test.ts | 32 +++++++++++++++++ .../src/datasource-admin-service.ts | 5 +-- .../src/datasource-connectivity-change.ts | 34 ++++++++++++++----- 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/.changeset/datasource-update-rebuilds-pool.md b/.changeset/datasource-update-rebuilds-pool.md index 5b95e7e510..3256be2f3c 100644 --- a/.changeset/datasource-update-rebuilds-pool.md +++ b/.changeset/datasource-update-rebuilds-pool.md @@ -1,5 +1,5 @@ --- -"@objectstack/service-datasource": patch +"@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 @@ -18,10 +18,10 @@ 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`, `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 + 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. 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 index a8e26d6110..33ef8a0efe 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts +++ b/packages/services/service-datasource/src/__tests__/datasource-update-rebuild.test.ts @@ -413,6 +413,38 @@ describe('datasourceConnectivityChanged — the ruled field set, read from what 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', diff --git a/packages/services/service-datasource/src/datasource-admin-service.ts b/packages/services/service-datasource/src/datasource-admin-service.ts index 79bc33e7ee..ba4305a958 100644 --- a/packages/services/service-datasource/src/datasource-admin-service.ts +++ b/packages/services/service-datasource/src/datasource-admin-service.ts @@ -492,8 +492,9 @@ export class DatasourceAdminService implements IDatasourceAdminService { // // Ruled decision tree (triage on the card): // - a connectivity-bearing field changed (`driver` / `config` / - // `external` incl. `credentialsRef` / `pool` / `active` — the reading - // behind the set lives on `datasourceConnectivityChanged`) → rebuild + // `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 diff --git a/packages/services/service-datasource/src/datasource-connectivity-change.ts b/packages/services/service-datasource/src/datasource-connectivity-change.ts index ac831257bd..a9044f2c7f 100644 --- a/packages/services/service-datasource/src/datasource-connectivity-change.ts +++ b/packages/services/service-datasource/src/datasource-connectivity-change.ts @@ -25,6 +25,10 @@ * 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 @@ -33,14 +37,21 @@ * `label` is read by nothing on the connect path — the reverse control: an * edit to it must not churn a working connection. * - * ⚠️ Known fork, reported on the card rather than resolved here: `schemaMode` - * is patchable on the update path AND is read by `attemptConnect` (policy - * gate, `toSpec` → `factory.create`, `registerDatasourceDef`), yet the ruled - * set omits it — so a schemaMode-only edit does not rebuild, and the engine's - * datasource def keeps the OLD schemaMode until restart. Deliberately not - * added: the ruling fixed the set, and widening it is the maintainer's call. - * (`ssl` is also a `toSpec` input but is not a field of `StoredDatasource` or - * `DatasourceDraft`, so it cannot change through this path.) + * 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'; @@ -48,7 +59,7 @@ 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' | 'active' + 'driver' | 'config' | 'external' | 'pool' | 'schemaMode' | 'active' >; /** @@ -61,6 +72,10 @@ export type ConnectivityBearingFields = Pick< * - 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, @@ -70,6 +85,7 @@ export function datasourceConnectivityChanged( 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; }