Skip to content

Commit 07585bf

Browse files
fix(service-datasource)!: reconnect requires previous, and the engine read-back answers the stored driver id (#27788)
<!-- claude-projects-attribution: {"github_login":"marchtian"} --> _Requested by **田騏** · [project thread](https://claude.ai/code/project/chan_01VmuUhXU9SSUzqJYyXSmYSi?thread=cmsg_01VmuUhXU9SSUzqJYyXSmYSiEe1mThB51wBSHAzPk8xmuG)_ **Fable review owed.** This PR was built on Opus while the Fable quota was out (maintainer order: finish on Opus, then wait for the contract review). It touches `packages/spec/src/**`, so it does not land until a `CONTRACT_REVIEW_TIER` contract review record PASS is on its current head. Fixes #27727 (epic #27261). The four should-fixes and the prose note from the contract review of PR #27715 (6508089206, ④ and ③). Claim 6509213861. ## What changes - `IDataEngine.registerDatasourceDef?` member doc (`packages/spec/src/contracts/data-engine.ts`): "last write wins, except an absent `driver` keeps the id stored before it", so the member doc and the `EngineDatasourceDef.driver` key doc no longer disagree. The engine's own `registerDatasourceDef` doc says the same. The key doc now also says the read-back answers the stored id. - `ObjectQL.listDatasourceDefs` answers each definition's stored `driver` id when it has one (chosen over documenting the omission: the id is what decides the floor, so a sweep asking which datasources name a plugin driver can now be written against the contract). A definition that never named a driver is listed without the key, as before; the existing `toEqual` pins carry no driver and are unchanged. - `DatasourceConnectionService.reconnect`: `opts.previous` is required (type level), and a call without `previous.driver` (absent or empty) is refused at runtime BEFORE anything is evicted or rebuilt, so the live pool keeps serving. Before this, a host calling `reconnect` without `previous` got a kept pool restored under a definition naming no driver, which serves a plugin driver unfloored, silently. Both in-repo callers (the admin plugin's `reregisterPool` and two test seams) already pass it. - `external-driver-floor.test.ts`: the #27277 keep test now names the caller class it models (a registration of the write-gate facts only) and says the connection service is not that caller. - `content/docs/data-modeling/virtual-objects.mdx`: the "ADR-0015 ... Tier A" line is gone (Tier A is epic #27261's term, not ADR-0015's); the ADR-0015 reference stays, describing what the ADR does govern on that page. ## Why Contract review 6508089206 ④ on #27666: no must-fix, and none of these opens a path today, but each leaves the next caller a way to write a datasource the floor does not protect, or a contract that says two things about one member. ## Verification Local, on `a5f0c8b5` (branch merged with `origin/main` at `34dc09b6`): - New pins, each red with the fix reverted and green with it (reverse-verified from a committed state): - objectql `external-driver-floor.test.ts`, `[#27727]` block: `listDatasourceDefs` answers the named id, the kept id, and the manifest-declared id (2 of 3 red without the engine change; the third is the control that a def with no driver is listed without the key). - service-datasource `datasource-connection-service.test.ts`, `[#27727]` block: a `reconnect` without `previous` (and with an empty `driver`) rejects, the live driver and the def list are untouched, and the factory built nothing (red without the guard); a failed rebuild restores the kept pool under the OLD driver id. A `@ts-expect-error` pins `previous` as required in a file the package's `tsc` compiles. - `pnpm --filter @objectstack/objectql test`: 9247/9247. `pnpm --filter @objectstack/service-datasource test`: 1013/1013. cli `secret-reference-union` and `absent-table-reads`: 49/49. - `typecheck` clean for objectql, service-datasource and spec; `pnpm lint` exit 0; spec `check:generated` exit 0 (all 14 artifacts current). - `dispatch-gates --ran`: 109 derived families, 109 run, 0 not measured, all exit 0 (after a full `pnpm build`). Clause-②: yes (narrowing) Metadata-protocol: none The claim declared `Clause-②: no (narrowing)` for the required `previous`; the diff also widens `listDatasourceDefs`'s answer by the stored `driver` key, so this PR and its changeset say `yes (narrowing)`, as the epic PM asked. The changeset carries the BREAKING banner (a host calling `reconnect` without `previous`; none in-repo) and the ADR-0087 marker `not-required (no-migration-prescription)`. ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed (doc-only change to a contract interface; `pnpm --filter @objectstack/spec check:generated` exits 0 against the freshly built dist, nothing to regenerate). - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. - [x] The *Before Submitting* list in CONTRIBUTING.md is done. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_017ihL2fhwqSBmy1XBXfcoU1 --- _Generated by [Claude Code](https://claude.ai/code/session_017ihL2fhwqSBmy1XBXfcoU1)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 04141a6 commit 07585bf

7 files changed

Lines changed: 144 additions & 14 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
'@objectstack/spec': patch
3+
'@objectstack/objectql': patch
4+
'@objectstack/service-datasource': patch
5+
---
6+
7+
fix(service-datasource): `DatasourceConnectionService.reconnect` requires `previous`, and the engine's datasource read-back answers the stored driver id (#27727)
8+
9+
Clause-②: yes (narrowing) — `DatasourceConnectionService.reconnect` now requires `opts.previous` with its `driver` id and refuses a call without it, and `ObjectQL.listDatasourceDefs` widens its answer by the stored `driver` key
10+
Metadata-protocol: none
11+
12+
<!-- adr-0087: not-required (no-migration-prescription) No key of any authored schema is added, removed, renamed or re-shaped: the narrowed parameter is on a host-facing service method, and the widened answer is an engine read-back. A datasource needs no metadata change, so there is nothing for a migration to rewrite. -->
13+
14+
**BREAKING** for a host that calls `DatasourceConnectionService.reconnect` itself without `previous`: the call is now a type error and, at runtime, rejects before anything is evicted. The in-repo caller (the datasource admin plugin's update path) already passes it.
15+
16+
The engine keeps a plugin driver on a non-managed datasource behind its correctness floor only while the stored datasource definition names the driver id. `reconnect` evicts the definition with the driver, and when a rebuild fails it restores the kept pool under a definition built from `previous`. Without `previous` that definition named no driver, so a kept plugin-driver pool was served without the floor, and nothing said so.
17+
18+
- `DatasourceConnectionService.reconnect`: `opts.previous` (the record the live pool was built from, including its `driver`) is required. A call without it, or with an empty `driver`, rejects with an error naming what to pass, and the live pool is left untouched.
19+
- `ObjectQL.listDatasourceDefs` answers each definition's stored `driver` id when it has one, so a sweep can ask which datasources name a plugin driver. A definition that never named one is still listed without the key.
20+
- `IDataEngine.registerDatasourceDef` (`@objectstack/spec/contracts`) now says what the engine already does: last write wins, except an absent `driver` keeps the id stored before it.
21+
22+
What to do: a host that calls `reconnect` directly passes the stored record as it was before the update as `previous`. Nothing else changes.

‎content/docs/data-modeling/virtual-objects.mdx‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,5 +631,4 @@ separate data-sync metadata type: a sync is defined on its target, as that mappi
631631
- [Hooks](/docs/automation/hooks) and [Hook bodies](/docs/automation/hook-bodies) — the
632632
`beforeUpdate` guard.
633633
- [Views](/docs/ui/views) — the gantt and every other list view.
634-
- ADR-0015 (federation spec) — the plugin driver is its Tier A: a driver, zero protocol
635-
change.
634+
- ADR-0015 (federation spec) — `schemaMode`, the `external` binding and the write gates.

‎packages/objectql/src/engine.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9244,7 +9244,8 @@ export class ObjectQL implements IObjectQLEngine {
92449244
* `external.allowWrites` so the write gate ({@link assertWriteAllowed}) can
92459245
* enforce external-datasource ownership, and `external.credentialsRef` so
92469246
* {@link listDatasourceDefs} can hand a credentials sweep the handle a
9247-
* code-declared datasource holds. Safe to call repeatedly; last write wins.
9247+
* code-declared datasource holds. Safe to call repeatedly; last write wins,
9248+
* except an absent `driver` keeps the id stored before it.
92489249
*/
92499250
registerDatasourceDef(def: DatasourceDef): void {
92509251
if (!def?.name) return;
@@ -9311,10 +9312,15 @@ export class ObjectQL implements IObjectQLEngine {
93119312
* Each entry is a fresh object with a COPIED `external` block. The index
93129313
* stores the caller's `external` by reference, and a reader must not be able
93139314
* to reach through this accessor and mutate the write gate's own input.
9315+
*
9316+
* [#27727] The stored `driver` id is answered too: it is what decides whether
9317+
* the datasource's driver is floored, so a sweep asking "which datasources
9318+
* name a plugin driver" can be written against the contract's read-back.
93149319
*/
93159320
listDatasourceDefs(): DatasourceDef[] {
93169321
return Array.from(this.datasourceDefs, ([name, def]) => ({
93179322
name,
9323+
...(def.driver !== undefined ? { driver: def.driver } : {}),
93189324
...(def.schemaMode !== undefined ? { schemaMode: def.schemaMode } : {}),
93199325
...(def.external ? { external: { ...def.external } } : {}),
93209326
}));

‎packages/objectql/src/external-driver-floor.test.ts‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -460,13 +460,15 @@ describe('[#27277] predicate writes are not offered: every write reaches the dri
460460
});
461461

462462
describe('[#27277] a re-registration that names no driver keeps the floor', () => {
463-
it('the datasource-connection sequence: def with driver, driver, then a def without driver', async () => {
463+
it('a caller registering only the write-gate facts: def with driver, driver, then a def without driver', async () => {
464464
const engine = new ObjectQL();
465465
const remote = remoteDriver('tree', TREE_ROWS);
466466
engine.registerDatasourceDef({ name: 'tree', driver: 'spike_tree', schemaMode: 'external', external: { allowWrites: true } });
467467
engine.registerDriver(remote.driver);
468-
// What `datasource-connection-service.ts` registers after a connect and in
469-
// `restoreOldPool`: no `driver` key (`EngineDatasourceDef` has none).
468+
// A caller that registers only the write-gate facts (`schemaMode` and
469+
// `external`) and names no `driver`. The datasource-connection service is
470+
// not that caller: it names `driver` on every registration (the [#27666]
471+
// block below).
470472
engine.registerDatasourceDef({ name: 'tree', schemaMode: 'external', external: { allowWrites: true } });
471473
engine.registerApp({
472474
id: 'tree_pkg',
@@ -584,3 +586,33 @@ describe('[#27666] the datasource-connection service\'s registrations floor a pl
584586
expect(engine.getDriverByName('tree')).toBe(rebuilt.driver);
585587
});
586588
});
589+
590+
describe('[#27727] listDatasourceDefs answers the stored driver id, the key the floor is decided on', () => {
591+
it('answers the id a registration named, and the id a later registration without one kept', () => {
592+
const engine = new ObjectQL();
593+
engine.registerDatasourceDef({ name: 'tree', driver: 'spike_tree', schemaMode: 'external', external: { allowWrites: true } });
594+
expect(engine.listDatasourceDefs()).toEqual([
595+
{ name: 'tree', driver: 'spike_tree', schemaMode: 'external', external: { allowWrites: true } },
596+
]);
597+
engine.registerDatasourceDef({ name: 'tree', schemaMode: 'external', external: { allowWrites: false } });
598+
expect(engine.listDatasourceDefs()).toEqual([
599+
{ name: 'tree', driver: 'spike_tree', schemaMode: 'external', external: { allowWrites: false } },
600+
]);
601+
});
602+
603+
it('answers the id a package manifest declared', () => {
604+
const engine = new ObjectQL();
605+
engine.registerApp({
606+
id: 'tree_pkg',
607+
name: 'Tree',
608+
datasources: [{ name: 'tree', driver: 'spike_tree', schemaMode: 'external', external: { allowWrites: true } }],
609+
} as any);
610+
expect(engine.listDatasourceDefs().find((d) => d.name === 'tree')?.driver).toBe('spike_tree');
611+
});
612+
613+
it('a definition that never named a driver is listed without the key, never as built in', () => {
614+
const engine = new ObjectQL();
615+
engine.registerDatasourceDef({ name: 'bare', schemaMode: 'external' });
616+
expect(engine.listDatasourceDefs()).toEqual([{ name: 'bare', schemaMode: 'external' }]);
617+
});
618+
});

‎packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1362,3 +1362,58 @@ describe('retained connection state (framework#3827)', () => {
13621362
]);
13631363
});
13641364
});
1365+
1366+
describe('[#27727] reconnect requires `previous`, the record the live pool was built from', () => {
1367+
const pluginDs: ConnectableDatasource = {
1368+
name: 'tree',
1369+
driver: 'spike_tree',
1370+
schemaMode: 'external',
1371+
external: { allowWrites: true },
1372+
};
1373+
const ctx = { origin: 'runtime', trigger: 'runtime-admin' } as const;
1374+
1375+
it('a call without `previous` is refused before anything is evicted or rebuilt', async () => {
1376+
const { service, engine, factory } = svc();
1377+
await service.connect(pluginDs, { context: ctx });
1378+
const live = engine!.drivers.get('tree');
1379+
expect(live).toBeDefined();
1380+
const defsBefore = engine!.defs.length;
1381+
1382+
// @ts-expect-error — `previous` is required: without its `driver` a kept pool would be restored unfloored
1383+
await expect(service.reconnect(pluginDs, { context: ctx })).rejects.toThrow(/reconnect needs `previous`/);
1384+
await expect(
1385+
service.reconnect(pluginDs, { context: ctx, previous: { driver: '' } }),
1386+
).rejects.toThrow(/with its `driver` id/);
1387+
1388+
expect(engine!.drivers.get('tree')).toBe(live);
1389+
expect(engine!.defs).toHaveLength(defsBefore);
1390+
expect(factory!.create).toHaveBeenCalledTimes(1);
1391+
});
1392+
1393+
it('with `previous`, a failed rebuild restores the kept pool under the OLD driver id', async () => {
1394+
const engine = fakeEngine();
1395+
let builds = 0;
1396+
const factory: IDatasourceDriverFactory = {
1397+
supports: () => true,
1398+
create: vi.fn(async () => {
1399+
builds += 1;
1400+
const driver: any = { name: 'com.fake.driver' };
1401+
return {
1402+
driver,
1403+
connect: builds === 1
1404+
? async () => { driver.connected = true; }
1405+
: async () => { throw new Error('connection refused'); },
1406+
};
1407+
}),
1408+
};
1409+
const { service } = svc({ engine, factory });
1410+
await service.connect(pluginDs, { context: ctx });
1411+
const live = engine.drivers.get('tree');
1412+
1413+
const next: ConnectableDatasource = { ...pluginDs, driver: 'sqlite', config: { filename: '/tmp/t.db' } };
1414+
await service.reconnect(next, { context: ctx, previous: pluginDs });
1415+
1416+
expect(engine.drivers.get('tree')).toBe(live);
1417+
expect(engine.defs[engine.defs.length - 1]).toMatchObject({ name: 'tree', driver: 'spike_tree', schemaMode: 'external' });
1418+
});
1419+
});

‎packages/services/service-datasource/src/datasource-connection-service.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,11 @@ export class DatasourceConnectionService {
825825
* `unregisterDriver` removes the datasource def together with the driver
826826
* (the registry owns that invariant), so the keep-old-pool path must restore
827827
* the def the old pool was serving under — otherwise the write gate would
828-
* judge writes for a datasource whose def vanished mid-failure.
828+
* judge writes for a datasource whose def vanished mid-failure. It is
829+
* REQUIRED ([#27727]): the restored def's `driver` is what keeps a kept
830+
* plugin-driver pool behind the engine's correctness floor, and a restore
831+
* that cannot name it would serve that pool unfloored, silently. A call
832+
* without `previous.driver` is refused before anything is evicted.
829833
*
830834
* Not for the DEFAULT datasource: runtime-admin records can never be named
831835
* `default` (the name is host-reserved), and the default driver's natural-
@@ -835,10 +839,18 @@ export class DatasourceConnectionService {
835839
record: ConnectableDatasource,
836840
opts: {
837841
context?: DatasourceConnectContext;
838-
previous?: Pick<ConnectableDatasource, 'driver' | 'schemaMode' | 'external'>;
839-
} = {},
842+
previous: Pick<ConnectableDatasource, 'driver' | 'schemaMode' | 'external'>;
843+
},
840844
): Promise<ConnectResult> {
841845
const name = record.name;
846+
if (typeof opts?.previous?.driver !== 'string' || opts.previous.driver === '') {
847+
throw new Error(
848+
`datasource '${name}': reconnect needs \`previous\`, the record the live pool was built from, ` +
849+
'with its `driver` id — without it a kept pool would be restored under a definition that names ' +
850+
'no driver, and a plugin driver would be served without the engine\'s correctness floor. ' +
851+
'Pass the stored record as it was before this update.',
852+
);
853+
}
842854
const engine = this.cfg.engine();
843855
const oldDriver = engine?.getDriverByName?.(name);
844856
if (!oldDriver || !engine || typeof engine.unregisterDriver !== 'function') {
@@ -859,9 +871,9 @@ export class DatasourceConnectionService {
859871
// plugin driver" and serve the kept pool unfloored.
860872
engine.registerDatasourceDef?.({
861873
name,
862-
driver: opts.previous?.driver,
863-
schemaMode: opts.previous?.schemaMode,
864-
external: opts.previous?.external as { allowWrites?: boolean } | undefined,
874+
driver: opts.previous.driver,
875+
schemaMode: opts.previous.schemaMode,
876+
external: opts.previous.external as { allowWrites?: boolean } | undefined,
865877
});
866878
};
867879
const stillServingSuffix =

‎packages/spec/src/contracts/data-engine.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,9 @@ export interface EngineDatasourceDef {
253253
* after a reconnect, whose eviction drops the stored def — read as "not a
254254
* plugin driver" and was served unfloored. Optional because a caller that
255255
* registers only the write-gate facts keeps the id stored before it; the
256-
* engine never reads an absent key as "built in".
256+
* engine never reads an absent key as "built in". {@link
257+
* IDataEngine.listDatasourceDefs} answers the stored id, so a sweep can ask
258+
* which datasources name a plugin driver.
257259
*/
258260
driver?: string;
259261
schemaMode?: string;
@@ -468,7 +470,9 @@ export interface IDataEngine {
468470
* — the engine accepts and retains the reference since #12758; before
469471
* this catch-up a caller typed against THIS contract was refused with
470472
* TS2353 for a value the runtime keeps). Distinct from registering a live
471-
* driver connection. Safe to call repeatedly; last write wins.
473+
* driver connection. Safe to call repeatedly; last write wins, except an
474+
* absent `driver` keeps the id stored before it (see
475+
* {@link EngineDatasourceDef.driver}).
472476
*/
473477
registerDatasourceDef?(def: EngineDatasourceDef): void;
474478

0 commit comments

Comments
 (0)