diff --git a/.changeset/driver-registry-eviction.md b/.changeset/driver-registry-eviction.md new file mode 100644 index 0000000000..c942c6335b --- /dev/null +++ b/.changeset/driver-registry-eviction.md @@ -0,0 +1,51 @@ +--- +"@objectstack/objectql": patch +"@objectstack/service-datasource": patch +"@objectstack/spec": minor +--- + +Deleting a datasource now evicts its driver from the data-engine registry, so `/api/v1/ready` recovers without a process restart + +**BREAKING** `IObjectQLEngine` gains a REQUIRED member, shipped as `minor` +under the repo's launch-window convention for breaking changes. + +`unregisterDriver(name: string): boolean` is additive for CONSUMERS — nothing +they already call changes — but it breaks any third-party *implementer* of +`IObjectQLEngine` at compile time, and the interface is on the published +surface (`packages/spec/src/contracts/index.ts` re-exports it and `./contracts` +is a published export path). Graded `minor` to match this contract's own +precedent: the three prior changes to it all took `minor`, including one that +added five members that were **all optional** and therefore broke nobody by +construction. A required member grading below that would be inconsistent. + +The ObjectQL driver registry had a `registerDriver` door and no counterpart, so +nothing could ever leave it. Deleting a datasource emptied the admin door while +`GET /api/v1/ready` kept naming the deleted datasource's driver — the readiness +probe reports whatever `checkDriversHealth()` finds in that registry — and on a +multi-replica deployment the only recovery was restarting every process. + +`IObjectQLEngine` gains `unregisterDriver(name)`, the removal counterpart of +`registerDriver`. The registry owns the invariant rather than each caller: an +eviction has to drop the driver entry, clear the `defaultDriver` NAME when it +pointed at the evicted driver (otherwise `getDefaultDriverName()` answers with a +name nothing backs), and drop the datasource definition that has no removal door +of its own. Evicting does not disconnect the pool — teardown belongs to whoever +owns it. + +Three lifecycle paths now use it: + +- **Datasource delete / pool teardown** — `DatasourceConnectionService.disconnect()` + evicts after closing the pool, which is the path `DELETE /api/v1/datasources/:name` + reaches. The default driver is evicted under its natural registered name. +- **Failed-start rollback** — a connect that throws after registering now rolls + that registration back, instead of leaving a driver the admin list reports as + failed and the readiness probe still pings. +- **Engine teardown** — `destroy()` disconnects and then evicts, so a destroyed + engine no longer reports drivers whose pools it has already closed. + +Eviction is per-replica, matching how driver registration already works +(each replica registers pools from the shared datasource records at boot); +propagating it cluster-wide would need a broadcast channel the driver registry +does not have today. + + diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 83862c32b9..ed4a084ea2 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -109,18 +109,18 @@ that silently does not happen. | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| -| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10712` | -| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10874` | -| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9605` | +| 18 | **`readonly` strip bypassed — UPDATE, single row** | objectql | Get: a `readonly` field CAN be written. Lose: the protection that stops a caller seeding e.g. `approval_status` | `objectql/src/engine.ts:10787` | +| 19 | **`readonly` strip bypassed — UPDATE, bulk/predicate** | objectql | Same, on the multi-row path | `objectql/src/engine.ts:10949` | +| 20 | **`readonly` strip bypassed — INSERT (engine pass)** | objectql | Same, on create | `objectql/src/engine.ts:9680` | | 21 | **`readonly` strip bypassed — INSERT (protocol ingress)** | metadata-protocol | `isSystem` is the **only** exemption here. `preserveAudit` is deliberately not read on this path (#6640) — a non-system historical import is still stripped on create | `metadata-protocol/src/protocol.ts:1664` | -| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9642`, `readonly-strict-errors.ts:66` | -| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5639` | +| 22 | Strict-drop refusal never fires | objectql | Lose: a caller that opted into loud refusal gets **silence** — strict refuses exactly what the strip would have taken, and the strip took nothing | `objectql/src/engine.ts:9717`, `readonly-strict-errors.ts:66` | +| 23 | **Referential-integrity check skipped** | objectql | Get: writes proceed against unreachable/unresolvable targets. Lose: an `isSystem` caller can write a **dangling reference** | `objectql/src/engine.ts:5705` | | 24 | Tenant-audit warning silenced; `bypassTenantAudit` threaded to the driver | objectql | Get: unscoped system writes stop warning. Lose: the signal that would flag a genuine user-path scoping bug | `objectql/src/engine.ts:3574`, `:3584`, `:3611` | | 25 | Engine-owned / append-only write guard bypassed | plugin-security | Get: generic writes to `managedBy` engine-owned objects | `system-write-guard.ts:96`, `:120` | | 26 | Identity write guard bypassed (ADR-0092) | plugin-auth | Get: direct writes to identity tables through the generic data path | `identity-write-guard.ts:98` | -| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6337` | -| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11460` | -| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11389` | +| 27 | Search-companion column **kept** in a read's rows when it was explicitly requested | objectql | Get: the internal companion column is readable. Lose: nothing for app code — this is the engine reading its own index | `objectql/src/engine.ts:6403` | +| 28 | Dependent-count disclosure on a blocked delete | objectql | Get: the count of blocking children. Nothing was elevated past the caller, so nothing is withheld | `objectql/src/engine.ts:11535` | +| 29 | Reference-cleanup log attributes the write to `'system'` | objectql | Get: an honest actor label instead of `anonymous` when the context carries neither `userId` nor `actor` | `objectql/src/engine.ts:11464` | ### 3. Sharing (`plugin-sharing`) @@ -180,7 +180,7 @@ a reader tracing where elevation travels needs them. | # | Site | Package | What it does | |:--|:---|:---|:---| | 62 | `objectql/src/engine.ts:3406` | objectql | Propagates `isSystem` into the hook session so hooks can tell engine self-writes from user writes | -| 63 | `objectql/src/engine.ts:13801` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | +| 63 | `objectql/src/engine.ts:13876` | objectql | `ScopedContext.isSystem` getter — re-exposes the underlying execution context's flag | | 64 | `plugin-reports/src/report-service.ts:556` | plugin-reports | Threads the flag into the engine call that runs a report | | 65 | `body-runner.ts:279` | runtime | Rebuilds an `ExecutionContext` from a hook session, carrying the flag across | @@ -195,7 +195,7 @@ assuming `isSystem` covers it is a documented source of bugs. |:---|:---|:---| | "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:1909` (rationale at `:1819`–`1821`, #3760), `flow.zod.ts:685` | | "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) | -| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9588`–`9605` | +| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:9663`–`9680` | | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1516` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:286` | diff --git a/packages/objectql/src/engine-driver-eviction.test.ts b/packages/objectql/src/engine-driver-eviction.test.ts new file mode 100644 index 0000000000..c1466cf84a --- /dev/null +++ b/packages/objectql/src/engine-driver-eviction.test.ts @@ -0,0 +1,217 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #13578 — the registry's missing removal door. +// +// The observed defect was operational: `DELETE /api/v1/datasources/:name` +// emptied the admin door on every replica while `GET /api/v1/ready` kept naming +// the deleted datasource's driver, recoverable only by restarting every +// process. The cause is here rather than at the probe — the driver registry had +// a `registerDriver` and no counterpart, so nothing could ever leave it. +// +// These pins hold the PRIMITIVE's invariants. The behavioural pin that the +// readiness probe actually stops naming an evicted datasource — driven through +// the real delete funnel — is +// `packages/runtime/src/registry-eviction-readiness.test.ts`, because only that +// package sees the engine, the connection service and the dispatcher at once. + +import { describe, it, expect } from 'vitest'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { ObjectQL } from './engine.js'; + +/** + * A registrable driver double, annotated `IDataDriver` for the reason the + * sibling `engine-primary-datasource.test.ts` fixture states: an un-annotated + * literal is checked only at the call site and drifts silently as the contract + * grows. + */ +const driver = (name: string, opts: { healthy?: boolean } = {}): IDataDriver => ({ + name, + version: '1.0.0', + supports: {}, + connect: async () => {}, + disconnect: async () => {}, + checkHealth: async () => opts.healthy !== false, + find: async () => [], + findOne: async () => null, + create: async (_o, data) => ({ id: '1', ...data }), + update: async (_o, id, data) => ({ id, ...data }), + upsert: async (_o, data) => ({ id: '1', ...data }), + delete: async () => true, + count: async () => 0, + bulkCreate: async () => [], + bulkUpdate: async () => [], + bulkDelete: async () => {}, + execute: async () => null, + beginTransaction: async () => ({}), + commit: async () => {}, + rollback: async () => {}, + syncSchema: async () => {}, + dropTable: async () => {}, +}); + +function newEngine(): ObjectQL { + return new ObjectQL({ logger: { debug() {}, info() {}, warn() {}, error() {} } } as any); +} + +function registerSys(engine: ObjectQL, name: string, extra: Record = {}): void { + engine.registry.registerObject( + { name, fields: { title: { type: 'text' } }, ...extra } as any, + 'platform-objects', + undefined, + 'own', + ); +} + +const healthNames = async (engine: ObjectQL) => + (await engine.checkDriversHealth()).map((r) => r.driverName).sort(); + +describe('#13578 — ObjectQL.unregisterDriver(), the registry\'s removal door', () => { + it('the entry the readiness probe reads is actually gone', async () => { + // The defect in one assertion: before this method existed, there was no + // call that could make `checkDriversHealth()` stop reporting a name. + const engine = newEngine(); + engine.registerDriver(driver('primary'), true); + engine.registerDriver(driver('stuck', { healthy: false })); + + expect(await healthNames(engine)).toEqual(['primary', 'stuck']); + + expect(engine.unregisterDriver('stuck')).toBe(true); + + expect(await healthNames(engine)).toEqual(['primary']); + expect(engine.getDriverByName('stuck')).toBeUndefined(); + }); + + it('evicts ONLY the named driver — the sibling entries are untouched', async () => { + // The positive control. An eviction that cleared the Map, or that keyed off + // the wrong name, would pass the assertion above and fail this one. + const engine = newEngine(); + engine.registerDriver(driver('keep_a'), true); + engine.registerDriver(driver('drop')); + engine.registerDriver(driver('keep_b')); + + engine.unregisterDriver('drop'); + + expect(await healthNames(engine)).toEqual(['keep_a', 'keep_b']); + expect(engine.getDriverByName('keep_a')).toBeDefined(); + expect(engine.getDriverByName('keep_b')).toBeDefined(); + }); + + it('returns false for a name the registry never held — a no-op is distinguishable', () => { + // An idempotent caller (a retried DELETE, a teardown sweep after a partial + // one) has to be able to tell "removed" from "there was nothing there"; + // a `void` return would have made both look identical. + const engine = newEngine(); + engine.registerDriver(driver('only'), true); + + expect(engine.unregisterDriver('never_registered')).toBe(false); + expect(engine.getDriverByName('only')).toBeDefined(); + }); + + describe('the invariants a caller could not have maintained itself', () => { + it('evicting the DEFAULT clears the default NAME, so nothing answers with a dead one', () => { + // `defaultDriver` is a name, not a reference. Deleting the Map entry + // alone would leave `getDefaultDriverName()` answering 'main' with + // nothing behind it — a worse state than the leak, because callers treat + // that answer as a live routing target. + const engine = newEngine(); + engine.registerDriver(driver('main'), true); + expect(engine.getDefaultDriverName()).toBe('main'); + + engine.unregisterDriver('main'); + + expect(engine.getDefaultDriverName()).toBeUndefined(); + }); + + it('after evicting the default, the #13408 primary verdict reads "cannot tell", never a name', () => { + // `engine-primary-datasource.test.ts` wrote this requirement down before + // eviction existed: "an eviction that removes the default is exactly how + // a registered system object stops being bound anywhere. When that lands, + // this must already read as 'cannot tell', not as a name." This is that + // reading, now driven by the real eviction rather than by an engine that + // never had a driver. + const engine = newEngine(); + engine.registerDriver(driver('sqlite'), true); + registerSys(engine, 'sys_user'); + expect(engine.resolvePrimaryDatasource()).toEqual({ + resolved: true, + datasource: 'sqlite', + witnesses: 1, + }); + + engine.unregisterDriver('sqlite'); + + // Unresolved ⇒ the readiness caller drains, which is the ruled + // fail-toward-draining direction. ⛔ Never `{ resolved: true, datasource: + // 'sqlite' }` read off a stale default. + expect(engine.resolvePrimaryDatasource()).toEqual({ + resolved: false, + reason: 'system-object-unbound', + }); + }); + + it('the datasource DEF goes with the driver — it has no removal door of its own', () => { + // `registerDatasourceDef` is public and there is no `unregisterDatasourceDef`, + // so a def outliving its driver is unreachable state: the write gate keeps + // judging writes against `external.allowWrites` for a datasource that no + // longer exists. + const engine = newEngine(); + engine.registerDriver(driver('warehouse')); + engine.registerDatasourceDef({ + name: 'warehouse', + schemaMode: 'external', + external: { allowWrites: false }, + } as any); + expect(engine.listDatasourceDefs().map((d) => d.name)).toContain('warehouse'); + + engine.unregisterDriver('warehouse'); + + expect(engine.listDatasourceDefs().map((d) => d.name)).not.toContain('warehouse'); + }); + + it('drops a def even when no driver was ever registered under the name', () => { + // A datasource that never connected leaves a def and no driver. Keying + // the def removal off "a driver was removed" would strand exactly the + // rows a FAILED datasource leaves behind — the population this card is + // about. + const engine = newEngine(); + engine.registerDatasourceDef({ name: 'never_connected', schemaMode: 'external' } as any); + expect(engine.listDatasourceDefs().map((d) => d.name)).toContain('never_connected'); + + expect(engine.unregisterDriver('never_connected')).toBe(false); + + expect(engine.listDatasourceDefs().map((d) => d.name)).not.toContain('never_connected'); + }); + }); + + describe('teardown is an eviction path too', () => { + it('destroy() leaves the registry EMPTY, not merely disconnected', async () => { + // Before #13578 `destroy()` disconnected every driver and left them all + // registered, so a destroyed engine still answered `checkDriversHealth()` + // by pinging pools it had just closed. + const engine = newEngine(); + engine.registerDriver(driver('a'), true); + engine.registerDriver(driver('b')); + expect(await healthNames(engine)).toEqual(['a', 'b']); + + await engine.destroy(); + + expect(await healthNames(engine)).toEqual([]); + expect(engine.getDefaultDriverName()).toBeUndefined(); + }); + + it('destroy() still disconnects every driver before evicting it', async () => { + // The ordering guard: evicting first would drop the only handle able to + // close the pool, turning a leak of registry entries into a leak of + // sockets. Eviction must not be a shortcut past teardown. + const closed: string[] = []; + const engine = newEngine(); + for (const name of ['a', 'b']) { + engine.registerDriver({ ...driver(name), disconnect: async () => { closed.push(name); } }); + } + + await engine.destroy(); + + expect(closed.sort()).toEqual(['a', 'b']); + }); + }); +}); diff --git a/packages/objectql/src/engine-primary-datasource.test.ts b/packages/objectql/src/engine-primary-datasource.test.ts index 8ba275ce7b..22b0f348d2 100644 --- a/packages/objectql/src/engine-primary-datasource.test.ts +++ b/packages/objectql/src/engine-primary-datasource.test.ts @@ -213,14 +213,16 @@ describe('ObjectQL.resolvePrimaryDatasource() — the #13408 criterion', () => { }); it('a registered system object bound nowhere at all ⇒ no answer can be true', () => { - // ⚠️ STRUCTURAL close, not a live production state today: `registerDriver` - // makes the FIRST driver the default (`isDefault || drivers.size === 1`), - // so step 5 always answers once any driver exists, and this branch is - // reachable only with none registered. It is pinned anyway because the - // engine has no driver eviction YET — adding it is #13578's half of this - // card — and an eviction that removes the default is exactly how a - // registered system object stops being bound anywhere. When that lands, - // this must already read as "cannot tell", not as a name. + // `registerDriver` makes the FIRST driver the default (`isDefault || + // drivers.size === 1`), so step 5 always answers once any driver exists + // and this branch needs none registered. + // + // No longer only structural: #13578 landed `unregisterDriver`, and an + // eviction that removes the default is exactly how a registered system + // object stops being bound anywhere. This reading — "cannot tell", never + // a name — is what that eviction was required to preserve, and it is now + // reached from the live direction too, in + // `engine-driver-eviction.test.ts`. const engine = newEngine(); registerSys(engine, 'sys_user'); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 390cae032e..6fe966c08a 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -5212,6 +5212,72 @@ export class ObjectQL implements IObjectQLEngine { } } + /** + * Evict a driver from the registry — the removal counterpart of + * {@link registerDriver} (#13578). + * + * ## Why the registry owns this rather than each lifecycle path + * + * #13578 named the fork explicitly: the delete path calling the registry, or + * the registry owning its own liveness. This is the second, and the reason is + * structural rather than stylistic — removing a driver is not one deletion + * but THREE pieces of private engine state that must move together, and a + * caller can reach none of them: + * + * 1. `drivers` — the Map {@link checkDriversHealth} iterates, and therefore + * the one a readiness probe reports. This is the entry #13578 watched + * survive a `DELETE /api/v1/datasources/:name` on every replica. + * 2. `defaultDriver` — a NAME, not a reference. Dropping the entry without + * clearing it leaves the default pointing at a driver that is gone, and + * {@link getDefaultDriverName} would answer with a name nothing backs. + * `engine-primary-datasource.test.ts` wrote the required reading down + * before this method existed: after an eviction that removes the default, + * the primary verdict must read "cannot tell", never a name. + * 3. `datasourceDefs` — the declarative `schemaMode` / `external` record + * {@link assertWriteAllowed} consults. It has a `registerDatasourceDef` + * door and no removal door at all, so a def outliving its driver keeps + * judging writes for a datasource that no longer exists. + * + * Only (1) is even visible from outside, and no public door removed it. So + * "every future lifecycle path remembers to clear three maps in the right + * order" is a rule with nowhere to live where it would be read — the shape + * this repo has already paid for. One primitive owns the invariant instead, + * and every path calls it exactly once. + * + * ⛔ Deliberately does NOT clear `unavailableDatasources`. That map already + * has its own door ({@link clearDatasourceUnavailable}) and its own caller, + * and the two acts are ordered OPPOSITELY on the failed-start path: a connect + * that fails after registering evicts the driver and then MARKS the + * datasource unavailable. Folding the clear in here would have eviction wipe + * the explanation the very next step writes. + * + * ⛔ Deliberately does NOT disconnect the driver. Eviction and teardown are + * separate acts: the pool's owner decides whether to close it — an ADOPTED + * host-owned instance outlives this kernel by design (ADR-0062 D5) — while + * the registry decides only whether this engine still routes to it. Folding + * them would make eviction close a pool the host still holds. + * + * @returns `true` when a driver entry was removed, `false` when the name held + * none — so an idempotent caller (a retried delete, a teardown sweep + * following a partial one) can tell a removal from a no-op. + */ + unregisterDriver(name: string): boolean { + const removed = this.drivers.delete(name); + // Cleared regardless of `removed`: a datasource that never connected has a + // def and no driver, so keying this off the driver entry would strand + // exactly the rows a failed datasource leaves behind. + this.datasourceDefs.delete(name); + if (this.defaultDriver === name) { + this.defaultDriver = null; + this.logger.info( + 'Evicted the DEFAULT driver — this engine has no default until one is registered', + { driverName: name }, + ); + } + if (removed) this.logger.info('Unregistered driver', { driverName: name }); + return removed; + } + /** * Register a Datasource *definition* (ADR-0015). * @@ -7783,6 +7849,15 @@ export class ObjectQL implements IObjectQLEngine { this.logger.error('Error disconnecting driver', e as Error, { driverName: name }); } } + + // #13578 — teardown is an eviction path too, and it used to disconnect + // every driver while leaving all of them REGISTERED. A destroyed engine + // therefore still answered `checkDriversHealth()` by pinging pools it had + // just closed, so a readiness probe racing shutdown read a list of drivers + // that were deliberately dead. Disconnect-then-evict, through the one + // primitive, so `defaultDriver` and the datasource defs go with them. + // Names are snapshotted first: `unregisterDriver` mutates the Map. + for (const name of [...this.drivers.keys()]) this.unregisterDriver(name); this.logger.info('ObjectQL engine destroyed'); } diff --git a/packages/runtime/src/registry-eviction-readiness.test.ts b/packages/runtime/src/registry-eviction-readiness.test.ts new file mode 100644 index 0000000000..d14b1ecc9e --- /dev/null +++ b/packages/runtime/src/registry-eviction-readiness.test.ts @@ -0,0 +1,184 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #13578 — the BEHAVIOURAL pin: after a datasource is deleted, `GET +// /api/v1/ready` stops naming its driver. +// +// The card was reported as an operational fact, not a structural one — "the +// admin-door list is empty on every replica … but `/ready` still names that +// datasource's driver … only a process restart clears it". A pin on +// `unregisterDriver()` alone would not have caught it: the defect was that +// nothing on the delete path CALLED the registry, and the probe reads the +// registry through two packages' worth of indirection. +// +// So this suite deliberately uses no doubles for the three things under test: +// +// - the REAL `ObjectQL` engine, which owns the driver registry; +// - the REAL `DatasourceConnectionService.disconnect()`, which is the funnel +// `DELETE /api/v1/datasources/:name` reaches via +// `removeDatasource` → `tryUnregisterPool` → `unregisterPool`; +// - the REAL `HttpDispatcher` `/ready` handler, which is the door the +// operator actually watched stay 503. +// +// `packages/runtime` is the only package that depends on all three, which is +// why the pin lives here and not beside either half. +// +// ⛔ The probe half is not re-pinned here — `http-dispatcher.ready.test.ts` +// owns the #13408 drain semantics, and this suite must not become a second +// opinion on them. What it pins is only that eviction reaches the probe. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { DatasourceConnectionService } from '@objectstack/service-datasource'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { HttpDispatcher, type HttpDispatcherResult } from './http-dispatcher.js'; + +/** See the sibling suites: annotated so the contract, not the call site, fails first. */ +const driver = (name: string, opts: { healthy?: boolean } = {}): IDataDriver => ({ + name, + version: '1.0.0', + supports: {}, + connect: async () => {}, + disconnect: async () => {}, + checkHealth: async () => opts.healthy !== false, + find: async () => [], + findOne: async () => null, + create: async (_o, data) => ({ id: '1', ...data }), + update: async (_o, id, data) => ({ id, ...data }), + upsert: async (_o, data) => ({ id: '1', ...data }), + delete: async () => true, + count: async () => 0, + bulkCreate: async () => [], + bulkUpdate: async () => [], + bulkDelete: async () => {}, + execute: async () => null, + beginTransaction: async () => ({}), + commit: async () => {}, + rollback: async () => {}, + syncSchema: async () => {}, + dropTable: async () => {}, +}); + +function newEngine(): ObjectQL { + return new ObjectQL({ logger: { debug() {}, info() {}, warn() {}, error() {} } } as any); +} + +function kernel(engine: unknown): any { + return { + getState: () => 'running', + getService: (name: string) => (name === 'data' ? engine : undefined), + getServiceAsync: async () => undefined, + }; +} + +/** + * `HttpDispatcherResult.response` is optional, so every read is a + * `possibly undefined` in a type-checked program — and this package's test + * layer IS type-checked by `check:type-check-debt`. Narrowed loudly, the shape + * lifted from `http-dispatcher.ready.test.ts` rather than invented again. + */ +function responseOf(res: HttpDispatcherResult): NonNullable { + const { response } = res; + if (!response) throw new Error('GET /ready answered no response at all'); + return response; +} + +/** What `/ready` says right now: the status, and every driver name it mentions. */ +async function ready(engine: unknown): Promise<{ status: number; named: string[] }> { + const res = await new HttpDispatcher(kernel(engine)).dispatch('GET', '/ready', undefined, undefined, {} as any); + const response = responseOf(res); + const body: any = response.body; + // Both shapes are read, deliberately: an unhealthy driver is named in + // `details.drivers` on the 503 and in `data.degraded.drivers` on the #13408 + // secondary-degraded 200. The card's symptom is "still NAMES it", so the + // assertion must not be able to pass merely because the envelope changed. + const named = [ + ...(body?.error?.details?.drivers ?? body?.details?.drivers ?? []), + ...(body?.data?.degraded?.drivers ?? []), + ]; + return { status: response.status, named: [...named].map(String).sort() }; +} + +/** The delete funnel, wired exactly as `datasource-admin-plugin` wires it. */ +function connectionServiceFor(engine: ObjectQL): DatasourceConnectionService { + return new DatasourceConnectionService({ + factory: () => undefined, + engine: () => engine, + }); +} + +describe('#13578 — DELETE of a datasource stops /ready naming its driver', () => { + it('the reported defect, end to end: after the delete funnel runs, /ready recovers without a restart', async () => { + const engine = newEngine(); + engine.registerDriver(driver('postgres_primary'), true); + engine.registerDriver(driver('stuck_mongo', { healthy: false })); + + // The state the operator observed: the app is healthy, one datasource's + // driver is stuck, and the probe names it. + const before = await ready(engine); + expect(before.status).toBe(503); + expect(before.named).toContain('stuck_mongo'); + + // `DELETE /api/v1/datasources/stuck_mongo` reaches exactly this call. + await connectionServiceFor(engine).disconnect('stuck_mongo'); + + // The recovery the card says only a process restart could produce. + const after = await ready(engine); + expect(after.named).not.toContain('stuck_mongo'); + expect(after.status).toBe(200); + }); + + it('POSITIVE CONTROL — a second stuck datasource is still named, and the healthy one survives', async () => { + // Without this, a fix that emptied the registry (or that made `/ready` + // stop reporting drivers at all) would pass the pin above. Deleting ONE + // datasource must recover exactly that one. + const engine = newEngine(); + engine.registerDriver(driver('postgres_primary'), true); + engine.registerDriver(driver('stuck_a', { healthy: false })); + engine.registerDriver(driver('stuck_b', { healthy: false })); + + expect((await ready(engine)).named).toEqual(['stuck_a', 'stuck_b']); + + await connectionServiceFor(engine).disconnect('stuck_a'); + + const after = await ready(engine); + expect(after.named).toEqual(['stuck_b']); + expect(after.status).toBe(503); + // The untouched datasources are still routable — eviction is not a reset. + expect(engine.getDriverByName('postgres_primary')).toBeDefined(); + expect(engine.getDriverByName('stuck_b')).toBeDefined(); + }); + + it('the pool is CLOSED as well as evicted — the delete does not leak the socket', async () => { + // Eviction must not become a shortcut past teardown: `disconnect()` + // resolves the driver out of the registry, so an eviction ordered before + // the close would drop the only handle that could close it. + let closed = false; + const engine = newEngine(); + engine.registerDriver(driver('primary'), true); + engine.registerDriver({ + ...driver('leaky', { healthy: false }), + disconnect: async () => { closed = true; }, + }); + + await connectionServiceFor(engine).disconnect('leaky'); + + expect(closed).toBe(true); + expect(engine.getDriverByName('leaky')).toBeUndefined(); + }); + + it('deleting the DEFAULT datasource evicts it under its NATURAL name', async () => { + // #3826: the default driver is registered under its own name, never under + // the literal 'default'. An eviction keyed on the datasource name would + // remove nothing here and report success — the exit-0-did-nothing shape. + const engine = newEngine(); + engine.registerDriver(driver('sqlite_main', { healthy: false }), true); + + expect((await ready(engine)).named).toContain('sqlite_main'); + + await connectionServiceFor(engine).disconnect('default', { asDefault: true }); + + expect((await ready(engine)).named).not.toContain('sqlite_main'); + expect(engine.getDriverByName('sqlite_main')).toBeUndefined(); + expect(engine.getDefaultDriverName()).toBeUndefined(); + }); +}); diff --git a/packages/services/service-datasource/src/__tests__/connection-engine-like-contract.test.ts b/packages/services/service-datasource/src/__tests__/connection-engine-like-contract.test.ts index fd436bb9d1..33854bbc03 100644 --- a/packages/services/service-datasource/src/__tests__/connection-engine-like-contract.test.ts +++ b/packages/services/service-datasource/src/__tests__/connection-engine-like-contract.test.ts @@ -19,6 +19,17 @@ import type { IDataDriver, IObjectQLEngine } from '@objectstack/spec/contracts'; import type { ConnectionEngineLike } from '../datasource-connection-service.js'; describe('ConnectionEngineLike is the contract, not a fork of it (#12010)', () => { + it('unregisterDriver answers a boolean — a no-op is distinguishable from a removal', () => { + // #13578. `void` would have made "evicted it" and "there was nothing to + // evict" indistinguishable to an idempotent caller (a retried DELETE, a + // teardown sweep after a partial one), which is the same + // exit-0-and-did-nothing shape the eviction fix exists to remove. + type Answer = ReturnType>; + const exact: [Answer] extends [boolean] ? ([boolean] extends [Answer] ? 'exact' : never) : never = + 'exact'; + expect(exact).toBe('exact'); + }); + it('the real engine contract is assignable to this view', () => { // The defect this card measured, stated as a compile: with // `registerDriver?: (driver: unknown, …)` the engine was NOT assignable @@ -63,10 +74,19 @@ describe('ConnectionEngineLike is the contract, not a fork of it (#12010)', () = expect(exact).toBe('exact'); }); - it('declares exactly the seven derived members, each identical to its contract member', () => { + it('declares exactly the eight derived members, each identical to its contract member', () => { + // #13578 added `unregisterDriver`, taking the roster from seven to eight. + // The roster is deliberately restated rather than derived from the seam: + // its whole purpose is that widening the view is a decision someone writes + // down here, not a side effect of editing the type. The eighth member is + // `registerDriver`'s removal counterpart — the service needs it because + // `disconnect()` has to evict the driver it just closed, and a datasource + // deleted from the admin door was otherwise left in the engine registry + // where the readiness probe kept reporting it. type Declared = keyof ConnectionEngineLike; type Expected = | 'registerDriver' + | 'unregisterDriver' | 'registerDatasourceDef' | 'getDriverByName' | 'syncObjectSchema' @@ -89,6 +109,7 @@ describe('ConnectionEngineLike is the contract, not a fork of it (#12010)', () = : never; const members: { [K in Expected]: Same } = { registerDriver: 'same', + unregisterDriver: 'same', registerDatasourceDef: 'same', getDriverByName: 'same', syncObjectSchema: 'same', @@ -98,7 +119,7 @@ describe('ConnectionEngineLike is the contract, not a fork of it (#12010)', () = }; expect(roster).toBe('exact'); - expect(Object.keys(members)).toHaveLength(7); + expect(Object.keys(members)).toHaveLength(8); }); it('every member stays OPTIONAL — the graceful-degradation seam', () => { diff --git a/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts b/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts index f63d96e958..14d34f5e7d 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts +++ b/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts @@ -61,6 +61,13 @@ function fakeEngine() { unavailable.delete(name); cleared.push(name); }, + // #13578 — the double implements the eviction door for the same reason it + // mirrors `registerDriver`'s skip-if-present above: a fake that silently + // lacks a member the seam declares turns every assertion about that member + // into a vacuous pass. `ConnectionEngineLike` is `Partial<…>`, so an absent + // `unregisterDriver` would make the optional call a no-op and the eviction + // pins below would hold against a service that evicted nothing. + unregisterDriver: (name) => drivers.delete(name), }; return engine; } @@ -991,6 +998,82 @@ describe('retained connection state (framework#3827)', () => { expect(engine!.unavailable.has('analytics')).toBe(false); }); + // ── #13578: the registry entries this service is responsible for removing ── + + it('#13578 — disconnect() EVICTS the driver, not merely the verdict', async () => { + // The reported defect at this seam. `disconnect()` closed the pool and + // dropped the retained state, but left the driver instance registered, so + // `checkDriversHealth()` — and through it `GET /api/v1/ready` — kept naming + // a datasource whose record the admin door had already deleted. + const { service, engine } = svc({ factory: fakeFactory() }); + await service.connect(analytics, { context: { trigger: 'runtime-admin' } }); + expect(engine!.drivers.has('analytics')).toBe(true); + + await service.disconnect('analytics'); + + expect(engine!.drivers.has('analytics')).toBe(false); + }); + + it('#13578 — evicts ONLY the named datasource', async () => { + // Positive control: an eviction that cleared the registry would pass the + // pin above and fail this one. + const { service, engine } = svc({ factory: fakeFactory() }); + await service.connect({ ...analytics, name: 'keep' }, { context: { trigger: 'runtime-admin' } }); + await service.connect({ ...analytics, name: 'drop' }, { context: { trigger: 'runtime-admin' } }); + + await service.disconnect('drop'); + + expect(engine!.drivers.has('drop')).toBe(false); + expect(engine!.drivers.has('keep')).toBe(true); + }); + + it('#13578 — a connect that throws AFTER registering rolls the registration back', async () => { + // The failed-start path the card asked to be walked. Registration happens + // partway through `attemptConnect`, so a throw after it used to return + // `failed-degraded` while leaving a live entry behind: a datasource the + // admin list reports as FAILED whose driver the readiness probe still pings + // — an orphan no door could reach. + const engine = fakeEngine(); + engine.registerDatasourceDef = () => { + throw new Error('boom — a step after registerDriver failed'); + }; + const service = new DatasourceConnectionService({ + factory: () => fakeFactory(), + engine: () => engine, + }); + + const result = await service.connect(analytics, { context: { trigger: 'runtime-admin' } }); + + expect(result.status).toBe('failed-degraded'); + expect(engine.drivers.has('analytics')).toBe(false); + // The rollback must NOT swallow the diagnosis: eviction deliberately leaves + // `unavailableDatasources` alone, and the mark is written after it. + expect(engine.unavailable.get('analytics')?.kind).toBe('failed'); + }); + + it('#13578 — the rollback never evicts a driver this attempt did not register', async () => { + // `registerDriver` keeps the incumbent when a name is already held, so + // "we called register" is not evidence that "we registered". Rolling back + // on that basis would let one datasource's failed connect evict another + // owner's live driver — a worse defect than the leak. + const engine = fakeEngine(); + engine.drivers.set('analytics', { name: 'analytics' }); + engine.registerDatasourceDef = () => { + throw new Error('boom'); + }; + const service = new DatasourceConnectionService({ + factory: () => fakeFactory(), + engine: () => engine, + }); + + // `already-registered` short-circuits before the build, so force the path + // by asking for the default-driver branch, whose guard tests for A default + // rather than for THIS name. + await service.connect(analytics, { context: { trigger: 'runtime-admin' }, asDefault: true }); + + expect(engine.drivers.get('analytics')).toEqual({ name: 'analytics' }); + }); + it('lists every retained verdict for the admin surface', async () => { const { service } = svc({ factory: fakeFactory({ connectThrows: true }) }); await service.connect({ ...analytics, name: 'a' }, { context: { trigger: 'runtime-admin' } }); diff --git a/packages/services/service-datasource/src/datasource-connection-service.ts b/packages/services/service-datasource/src/datasource-connection-service.ts index 9d6d966dbf..116bba6591 100644 --- a/packages/services/service-datasource/src/datasource-connection-service.ts +++ b/packages/services/service-datasource/src/datasource-connection-service.ts @@ -135,6 +135,7 @@ export type ConnectionEngineLike = Partial< | 'getDefaultDriverName' | 'markDatasourceUnavailable' | 'clearDatasourceUnavailable' + | 'unregisterDriver' > >; @@ -587,6 +588,13 @@ export class DatasourceConnectionService { } } + // #13578 — set to the registry key ONLY when this attempt is the thing that + // put it there, so the rollback below can never evict a driver someone else + // owns. `registerDriver` is a no-op on a name it already holds (it keeps the + // incumbent and discards the newcomer), and the `asDefault` branch's + // idempotency guard above tests for A default rather than for THIS name — so + // "we called register" is not evidence that "we registered". + let registeredByThisAttempt: string | undefined; try { const handle = await factory.create({ ...toSpec(record), ...(secret ? { secret } : {}) }); if (typeof handle?.connect === 'function') await handle.connect(); @@ -615,7 +623,17 @@ export class DatasourceConnectionService { /* frozen driver — registration may still work if name already matches */ } } + // Fails SAFE when the engine cannot answer: an engine without + // `getDriverByName` (the seam is `Partial<…>`) leaves us unable to tell + // our registration from someone else's, and evicting on a guess is the + // worse error of the two — so assume it was already held and roll nothing + // back. + const heldBefore = + typeof engine.getDriverByName === 'function' + ? engine.getDriverByName(engineDriver.name) !== undefined + : true; engine.registerDriver(engineDriver, opts.asDefault === true); + if (!heldBefore) registeredByThisAttempt = engineDriver.name; engine.registerDatasourceDef?.({ name, schemaMode: record.schemaMode, @@ -645,6 +663,18 @@ export class DatasourceConnectionService { this.logger?.info?.(`datasource '${name}': connected (driver=${record.driver}, schemaMode=${record.schemaMode ?? 'managed'})`); return { name, status: 'connected', ...(handle.ownership ? { ownership: handle.ownership } : {}) }; } catch (err) { + // #13578 — failed-start ROLLBACK, the first path the card asked to be + // walked. Registration happens partway through this block, so a throw + // after it used to return `failed-degraded` while leaving a live entry in + // the registry: a datasource the admin list reports as failed, whose + // driver the readiness probe still pings. Narrow today (the steps after + // `registerDriver` are individually guarded), but the window is real and + // the rollback is what makes "failed ⇒ not registered" true by + // construction rather than by the current arrangement of the lines. + // + // Before `handleFailure`, which records the unavailable mark: eviction + // deliberately does not touch that map, and the mark must outlive it. + if (registeredByThisAttempt) engine.unregisterDriver?.(registeredByThisAttempt); // `err` itself is handed on, not just its message: the driver package's // build output being absent is reported as `err.code === // 'ERR_MODULE_NOT_FOUND'`, and that structured signal is gone the moment @@ -686,6 +716,22 @@ export class DatasourceConnectionService { // datasource that no longer exists in that state. this.states.delete(name); engine?.clearDatasourceUnavailable?.(name); + + // #13578 — the eviction this method never did. Closing the pool above only + // ends the CONNECTION; the driver instance stayed in the engine registry, + // so `checkDriversHealth()` (and through it `GET /api/v1/ready`) kept + // naming a datasource whose record had already been deleted, with a process + // restart as the only recovery. + // + // It goes LAST, and after the disconnect: `driver` above is resolved out of + // the registry, so evicting first would leave the pool open with nothing + // left holding a handle to close it. + // + // `driverName` rather than `name`: the DEFAULT driver is registered under + // its NATURAL name (#3826), so `name` ('default') matches no registry key + // and evicting by it would silently remove nothing — the same + // exit-0-and-did-nothing shape this fix exists to remove. + if (driverName) engine?.unregisterDriver?.(driverName); } /** diff --git a/packages/spec/src/contracts/objectql-engine.ts b/packages/spec/src/contracts/objectql-engine.ts index e64619453c..11e1d15fb5 100644 --- a/packages/spec/src/contracts/objectql-engine.ts +++ b/packages/spec/src/contracts/objectql-engine.ts @@ -306,6 +306,20 @@ export interface IObjectQLEngine extends IDataEngine { // ── Boot-time wiring (AppPlugin / metadata-protocol) ───────────────── /** Register a driver; the optional second argument makes it the default. */ registerDriver(driver: IDataDriver, isDefault?: boolean): void; + /** + * Evict a driver from the registry — `registerDriver`'s removal counterpart + * (#13578). Returns `true` when an entry was removed. + * + * Declared here because its ABSENCE was the defect: the registry had a + * registration door and no eviction door, so a datasource deleted through + * the admin API left its driver instance registered and the readiness probe + * (`checkDriversHealth`) kept reporting it until the process restarted. + * + * Evicting only stops this engine routing to the driver. It does NOT + * disconnect the pool — teardown belongs to whoever owns the pool, which + * for an adopted host-owned instance is not this engine (ADR-0062 D5). + */ + unregisterDriver(name: string): boolean; /** Install the stack's datasource-mapping rules. Rule shape is engine-local; see `setDatasourceMapping` on the class. */ setDatasourceMapping(rules: unknown[]): void; /** Register an app/plugin manifest (objects, apps, metadata items) — MetadataProtocolPlugin's table-provisioning path. */