Skip to content

Commit 94cef75

Browse files
fix(service-datasource): Test connection answers ok:false when the driver's health check answers false (#30407) (#30566)
domain:server#3 · os-bill · session_01Utp6CXKK9SECcrtZHzxjTK <!-- claude-projects-attribution: {"github_login":"os-bill"} --> _Requested by **Bill** · [project thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wgSeyJbqJn9JhDcpyKRreAni)_ Fixes #30407 Clause-②: no — a served answer comes to match the driver's own health answer; no accept set changes. Metadata-protocol: none ## 维护者速读 - 改了什么:数据源「测试连接」现在读取驱动健康检查的返回值;连接成功但健康检查答 false 时,答 `ok: false` 并给出一句说明,不再答成功。 - 为什么改:探测只 await 了 `ping`(即驱动的 `checkHealth()`)却丢掉了它的布尔值,一个只告警不抛错、随后自报不健康的连接会被报成「连接成功」。 - 风险与回滚:只改 `datasource-admin-plugin.ts` 的 probe 一处判断;只有字面量 `false` 才拒,抛错路径与 ping 无返回值的句柄行为不变;回滚即 revert 本 PR。 - 先看:`packages/services/service-datasource/src/datasource-admin-plugin.ts` 的 probe(`health === false` 那一行);其测试文件末尾新增的 describe。 - 两分钟验证:`pnpm --filter @objectstack/service-datasource exec vitest run src/__tests__/datasource-admin-plugin.test.ts -t "probe reads the health answer"`,预期 4 条全绿。 - 需要你做的:无,席位落地。 ## What changes `DatasourceAdminServicePlugin.probe` awaited the handle's `ping` (or the `checkHealth` fallback) and dropped the answer. Both the shared factory's `toHandle` and `createPrebuiltDriverFactory` map `ping` to the driver's own `checkHealth()`, which reports an unhealthy connection by answering `false` rather than throwing, so a connect that resolved and then reported itself unhealthy answered Test connection `ok: true`. The probe now keeps that answer and, when it is a literal `false`, returns `ok: false` with the sentence "The driver connected, but its health check reported the database as unhealthy, so this connection is not usable. Check that the database exists and accepts these settings, then test again." The teardown in `finally` still runs. Only `false` refuses: `ping` is typed as a promise of `unknown` on the handle contract, and a host-built handle whose ping answers nothing keeps answering `ok: true` (the existing `fakeFactory` probe test pins that). The thrown-error path is unchanged. **Other callers of the handle's `ping`:** none. `git grep` over `packages/**` and `ui/**` (non-test) finds the probe as the only reader of a `DatasourceDriverHandle.ping`. The neighbours read the boolean already: `ExternalDatasourceService.testConnection` (saved-datasource Test) checks `await driver.checkHealth()` and answers `ok: false` on `false`; `engine-lifecycle-health.ts` maps `false` to `healthy: false`. `packages/runtime/src/turso-driver-factory.ts` and `prebuilt-driver-factory.ts` are producers of `ping`, not readers. **Serial hold:** the card was serial after PR #30395 (same package). Seat 3 lifted the hold at 15:00Z because this diff shares no file with #30395; #30395 then merged (22f0d65) and `origin/main` is merged into this branch before opening, so the PR is measured against a tree that already contains it. ## Tests New top-level `describe('DatasourceAdminServicePlugin: probe reads the health answer')` in the existing harness `src/__tests__/datasource-admin-plugin.test.ts`: - a stub DRIVER (connect resolves, `checkHealth()` answers `false`) wrapped through `createPrebuiltDriverFactory`, the producer that mirrors `toHandle`: `ok: false`, the error names the health check, no `latencyMs`, and connect / checkHealth / disconnect each ran once; - a handle carrying only `checkHealth` (no `ping`) answering `false`: `ok: false`; - CONTROL: the same stub with `checkHealth()` answering `true`: `ok: true` with a latency; - CONTROL: a throwing connect keeps its answer exactly (`{ ok: false, error: THROWN_MESSAGE }`), health never asked, teardown still ran. Measured first, on `fa9c0aa2` before any source edit: the two `false` pins red (`expected true to be false`), both controls green. Ablation (one, on the committed fix `2cc82972`): `scripts/ablation-replace.mjs --delete` on the `if (health === false) return …` line, anchor x1 → x0, blob 9c49cde → ae6fa722; the file run went `2 failed | 33 passed (35)` (exactly the two `false` pins), then restored to blob == HEAD with `git diff HEAD` empty. The suite imports the plugin by relative `src` path, so no `dist` leg applies. ## Acceptance notes - The card's production path (`resolveSqliteDriver` handing back a native driver whose `connect()` swallows a code-less error) is latent after PR #30395, as the triage says; this PR closes the reading for every driver that warns instead of throwing. ## Verification All on head `28086960` (the fix `2cc82972` plus a merge of `origin/main` `8e5fabe5`, which contains PR #30395). - `pnpm --workspace-concurrency=2 --filter '@objectstack/service-datasource...' build && pnpm --filter @objectstack/service-datasource typecheck && pnpm --filter @objectstack/service-datasource test` under `os-verify-lock.sh`: `VERDICT command-exit 0`; `Test Files 72 passed (72)`, `Tests 1187 passed (1187)`. - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`: 59 derived families, each run and its exit recorded; `--ran` reconciliation: `59 derived famil(ies) accounted for — 58 run, 1 NOT-MEASURED`. - 58 exit 0. `check-plugin-teardown-shape.mjs --self-test` first exited 3 on the shallow clone (fixture commit absent); after `git fetch origin 621a487…` it exited 0 (48 cases pass). - NOT MEASURED: `pnpm check:dual-build-cjs-loads`, reason: exit 3 PREREQUISITE NOT MET, it reads every package's `dist/` and only the service-datasource closure was built here. CI runs it in `Lint & Repo Gates`. - `pnpm --filter @objectstack/spec check:liveness`: exit 0. - Not run locally, declared to CI: `test:repo` (no file of that project changed), repo-wide `pnpm lint`, the full farm. --- _Generated by [Claude Code](https://claude.ai/code/session_01Utp6CXKK9SECcrtZHzxjTK)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7a5f73e commit 94cef75

3 files changed

Lines changed: 95 additions & 2 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@objectstack/service-datasource': patch
3+
---
4+
5+
fix(service-datasource): Test connection answers `ok: false` when the driver connects but its own health check answers `false` (#30407)
6+
7+
Clause-②: no — a served answer comes to match the driver's own health answer; no accept set changes.
8+
Metadata-protocol: none
9+
10+
The datasource probe awaited the handle's `ping` (the driver's `checkHealth()`) and dropped its boolean, so a connect that only warned and then reported itself unhealthy answered Test connection `ok: true`. The probe now reads that answer: a literal `false` answers `ok: false` with a sentence saying the driver connected but its health check reported the database as unhealthy. A connect or ping that throws keeps answering the thrown message, and a handle whose `ping` answers nothing keeps answering `ok: true`.

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
type DatasourceAdminServicePluginOptions,
1010
} from '../datasource-admin-plugin.js';
1111
import { registerDatasourceAdminRoutes } from '../admin-routes.js';
12+
import { createPrebuiltDriverFactory } from '../prebuilt-driver-factory.js';
1213
import { ENTITLED_CREDENTIAL, createSessionAuthService, createGrantsEngine } from './entitled-caller.fixture.js';
1314

1415
import { assertEngineFindOnePredicate, hashSpec } from '@objectstack/metadata-core';
@@ -855,3 +856,64 @@ describe('DatasourceAdminServicePlugin: objects bound external are read from the
855856
expect(registry.get('datasource')?.has('wh')).toBe(true);
856857
});
857858
});
859+
860+
// The probe reads the driver's own health answer: a connect that resolves but a
861+
// health check answering `false` is not a usable connection, so Test connection
862+
// must not answer `ok: true` over it. The stub is a DRIVER (connect resolves,
863+
// checkHealth answers a boolean) wrapped the way production wraps one — through
864+
// `createPrebuiltDriverFactory`, whose handle mirrors the shared factory's
865+
// `toHandle` (`ping` and `checkHealth` both answer the driver's checkHealth()).
866+
describe('DatasourceAdminServicePlugin: probe reads the health answer', () => {
867+
function stubDriver(health: boolean, over: { connect?: () => Promise<void> } = {}) {
868+
const calls = { connect: 0, checkHealth: 0, disconnect: 0 };
869+
const driver = {
870+
name: 'stub',
871+
connect: over.connect ?? (async () => { calls.connect += 1; }),
872+
checkHealth: async () => { calls.checkHealth += 1; return health; },
873+
disconnect: async () => { calls.disconnect += 1; },
874+
};
875+
return { driver, calls };
876+
}
877+
const draft = { name: 'reporting', driver: 'postgres', config: { host: 'db', database: 'analytics' } };
878+
879+
it('answers ok:false with a refusal sentence when connect resolves and the health check answers false', async () => {
880+
const { driver, calls } = stubDriver(false);
881+
const { service } = await boot({ driverFactory: createPrebuiltDriverFactory(driver, { driverId: 'postgres' }) });
882+
const res = await service.testConnection(draft);
883+
expect(res.ok).toBe(false);
884+
expect(res.error).toMatch(/health check/i);
885+
expect(res.latencyMs).toBeUndefined();
886+
expect(calls).toEqual({ connect: 1, checkHealth: 1, disconnect: 1 });
887+
});
888+
889+
it('answers ok:false when the handle carries only checkHealth (no ping) and it answers false', async () => {
890+
const { service } = await boot({
891+
driverFactory: fakeFactory({
892+
create: async () => ({ connect: async () => {}, checkHealth: async () => false, disconnect: async () => {} }),
893+
}),
894+
});
895+
const res = await service.testConnection(draft);
896+
expect(res.ok).toBe(false);
897+
expect(res.error).toMatch(/health check/i);
898+
});
899+
900+
it('CONTROL: a healthy stub driver still answers ok:true', async () => {
901+
const { driver, calls } = stubDriver(true);
902+
const { service } = await boot({ driverFactory: createPrebuiltDriverFactory(driver, { driverId: 'postgres' }) });
903+
const res = await service.testConnection(draft);
904+
expect(res.ok).toBe(true);
905+
expect(typeof res.latencyMs).toBe('number');
906+
expect(calls).toEqual({ connect: 1, checkHealth: 1, disconnect: 1 });
907+
});
908+
909+
it('CONTROL: a throwing connect keeps its answer (the thrown message, health never asked)', async () => {
910+
const { driver, calls } = stubDriver(true, {
911+
connect: async () => { throw new Error('connect ECONNREFUSED 10.0.0.5:5432'); },
912+
});
913+
const { service } = await boot({ driverFactory: createPrebuiltDriverFactory(driver, { driverId: 'postgres' }) });
914+
const res = await service.testConnection(draft);
915+
expect(res).toEqual({ ok: false, error: 'connect ECONNREFUSED 10.0.0.5:5432' });
916+
expect(calls.checkHealth).toBe(0);
917+
expect(calls.disconnect).toBe(1);
918+
});
919+
});

‎packages/services/service-datasource/src/datasource-admin-plugin.ts‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,9 +1016,20 @@ export class DatasourceAdminServicePlugin implements Plugin {
10161016
if (typeof driver?.connect === 'function') await driver.connect();
10171017
// Prefer a cheap ping; fall back to the engine driver's health check, then
10181018
// a schema introspection round-trip — whichever the handle exposes.
1019-
if (typeof driver?.ping === 'function') await driver.ping();
1020-
else if (typeof driver?.checkHealth === 'function') await driver.checkHealth();
1019+
//
1020+
// [#30407] The answer is read, not only awaited: the shared factory's
1021+
// `toHandle` (and `createPrebuiltDriverFactory`) map `ping` to the
1022+
// driver's own `checkHealth()`, which reports an unhealthy connection by
1023+
// answering `false` rather than throwing. A connect that only warns and
1024+
// then reports itself unhealthy answered Test connection `ok: true`.
1025+
// Only a literal `false` refuses: `ping` is `Promise<unknown>` on the
1026+
// contract, and a host-built handle's ping that answers nothing (or any
1027+
// other value) keeps meaning "the round-trip succeeded".
1028+
let health: unknown;
1029+
if (typeof driver?.ping === 'function') health = await driver.ping();
1030+
else if (typeof driver?.checkHealth === 'function') health = await driver.checkHealth();
10211031
else if (typeof driver?.introspectSchema === 'function') await driver.introspectSchema();
1032+
if (health === false) return { ok: false, error: UNHEALTHY_PROBE_REFUSAL };
10221033
const latencyMs = elapsedSince(startedAt);
10231034
let serverVersion: string | undefined;
10241035
try {
@@ -1039,6 +1050,16 @@ export class DatasourceAdminServicePlugin implements Plugin {
10391050
}
10401051
}
10411052

1053+
/**
1054+
* [#30407] What Test connection answers when the driver connected but its own
1055+
* health check answered `false`. The thrown-error path keeps answering the
1056+
* driver's message; this is the one answer for a health check that reports
1057+
* failure without throwing.
1058+
*/
1059+
const UNHEALTHY_PROBE_REFUSAL =
1060+
'The driver connected, but its health check reported the database as unhealthy, so this '
1061+
+ 'connection is not usable. Check that the database exists and accepts these settings, then test again.';
1062+
10421063
function safeGetService<T>(ctx: PluginContext, name: string): T | undefined {
10431064
try {
10441065
return ctx.getService<T>(name);

0 commit comments

Comments
 (0)