Skip to content

Commit a0f8654

Browse files
fix(service-datasource): an unset env credential is a note after the driver's error, and a failed connect holds no claim on its variable (#28283) (#28325)
<!-- claude-projects-attribution: {"github_login":"os-project-manager"} --> _Requested by **Leehom** · [project thread](https://claude.ai/code/project/chan_01TenzsRopZj4jMbvjX8uz1X?thread=cmsg_01TenzsRopZj4jMbvjX8uz1XL1T1xRb3yMBHrxcdDdxwnE)_ Fixes #28283 Clause-②: no — measured: the connect verdict's `reason` text changes shape and an internal owner map releases on failure. No schema, export, type, error code or status moves, nothing is newly refused or accepted, and no consumer outside this package's own pins reads the sentence (`git grep "not set on this host"` outside `service-datasource`: 0 hits). Metadata-protocol: none ## 维护者速读 - **改了什么**:代码里声明的数据源在环境变量 `OS_DATASOURCE_NAME_PASSWORD` 没设、连接又失败时,失败原因改为「驱动自己的错误 + 结尾的 `(note: …变量没设…)`」,不再用破折号把变量说成失败的原因;另外,一次失败的连接会释放它登记的变量,之后另一个推导出同一变量的数据源不会再被误判为冲突。 - **为什么改**:#28208 的契约评审 S1/S2:主机不可达、驱动模块缺失、TLS 失败时,运维被引去先设密码;失败的连接还一直占着冲突登记,直到 `disconnect`。 - **风险与回滚**:低。只改 `datasource-connection-service.ts` 里 env 凭据槽的两处:原因文本的拼接,和失败路径上释放本次登记。两个都活着的同变量数据源仍被拒绝。回滚即 revert。 - **先看**:`packages/services/service-datasource/src/datasource-connection-service.ts`(`claimedVariable` 的登记与 catch 里的释放、`(note: …)` 那一句);`src/__tests__/datasource-env-credential.test.ts`(两条新 pin)。 - **两分钟验证**:`pnpm --filter @objectstack/service-datasource exec vitest run --maxWorkers=2 src/__tests__/datasource-env-credential.test.ts`,预期 26/26 通过。 - **需要你做的**:无,席位落地。 ## What changes Both items are in `DatasourceConnectionService.connect()`'s env-credential slot (#27687): - **S1, the reason.** With the slot applied and the variable unset, a failed build used to answer `DRIVER_ERROR — datasource 'x' is defined in code and reads its credential from OS_DATASOURCE_X_PASSWORD at connect, which is not set on this host`. The base already led with the driver's error, so the order's hypothesis that the reason led with the variable did not hold as written. The dash joined the sentence on as the explanation, though, for every cause: an unreachable host, a missing driver module, a TLS failure. It now reads `DRIVER_ERROR (note: datasource 'x' is defined in code and reads its credential from OS_DATASOURCE_X_PASSWORD at connect, which is not set on this host)`. The note stays for a login refusal, so that pin still names the variable to set. I did not take the card's other route, matching the driver's error for "reads as an authentication refusal": that needs a per-driver table of auth codes (pg `28P01`, mysql `1045`, mssql `ELOGIN`…) that would drift silently, and the note is true for every cause. - **S2, the owner.** The collision check and the claim stay one step, before the build, with no await between them. `claimedVariable` records that THIS attempt made the claim, and the `failed-degraded` catch releases it before `handleFailure`, which may throw the fail-fast error. A connect of a name that already owned the variable claims nothing new and releases nothing. I kept the claim before the build, rather than recording it after the build succeeds, so that two overlapping connects of colliding names cannot both pass the check across the build's awaits. - A `patch` changeset for `@objectstack/service-datasource`. The docs line in `external-datasources.mdx` ("its reason names the variable to set") stays true and is not edited. ## Tests `src/__tests__/datasource-env-credential.test.ts`, through the real `DatasourceConnectionService`: - **S1 pin (new):** a code datasource, slot applied, variable unset, whose connect throws `connect ECONNREFUSED 127.0.0.1:1` (`code: 'ECONNREFUSED'`). The reason starts with the driver's error, the rest is exactly ` (note: … OS_DATASOURCE_WAREHOUSE_PASSWORD …)`, and the retained connection state carries the same reason. - **S1 control (unchanged):** "the database refusing the login answers the variable to set" passes. - **S2 pin (new):** `eu-sales` fails `failed-degraded` (`getaddrinfo ENOTFOUND`), then `eu_sales`, which derives the same `OS_DATASOURCE_EU_SALES_PASSWORD`, connects. - **S2 control (unchanged):** "the second name is refused with both names and the variable" (both live) passes. <details><summary>Red first, green, and one ablation per pin</summary> - Base, the fix not yet applied: `Tests 2 failed | 24 passed (26)`. S1: `expected ' — datasource \'warehouse\' is define…' to match /^ \(note: …/`. S2: `expected 'failed-credentials' to be 'connected'`. - Head: `Tests 26 passed (26)`. - Ablation 1, through `scripts/ablation-replace.mjs`, restores `} — datasource '${name}' is defined`. On disk: note-count 0, dash-count 1. Result: only the S1 pin fails (`1 failed | 25 passed`). Restored: blob == HEAD `2753417323b5`, `git diff HEAD` empty. - Ablation 2 deletes `if (claimedVariable !== undefined) this.envCredentialOwners.delete(claimedVariable);`. On disk: release-count 0. Result: only the S2 pin fails (`1 failed | 25 passed`). Restored: blob == HEAD, `git diff HEAD` empty. - The suite imports `../datasource-connection-service.js` from `src/`, so no `dist/` rebuild sits in either leg. </details> <details><summary>Package suite, typecheck, lint</summary> - `pnpm --filter @objectstack/service-datasource test`: `Test Files 66 passed (66)`, `Tests 1100 passed (1100)`, lock VERDICT command-exit 0. - `pnpm --filter @objectstack/service-datasource typecheck`: `tsc --noEmit`, VERDICT command-exit 0. `--listFiles` includes the test file. - `eslint src --no-inline-config` in the package: exit 0. </details> ## Acceptance notes - Security: no credential value appears in a test, log or this body. The pins use variable NAMES and a fixed fake value, and the existing no-leak pin passes. - The reason is operator-facing free text: the retained connect verdict (Setup → Datasources `statusReason`, the saved-datasource Test answer) and the fail-fast boot message. It is pinned only by this package's tests, and only by `toContain` on the variable name, which still holds. - `packages/runtime`'s `datasource-env-credential.test.ts` reads this package from `dist/` and asserts no reason text. No export changes, so per the local-verification scope it is left to CI's Test Core. ## Verification Gates from `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` on this diff (60 commands), run on head `d3ceca42` with each exit code captured before any pipe: 59 exit 0. `pnpm check:dual-build-cjs-loads` exits 3, PREREQUISITE NOT MET (a fresh worktree has not built every package's `dist`), so it is NOT MEASURED locally and left to CI. `check-plugin-teardown-shape.mjs --self-test` first exited 3 on the shallow clone, and exits 0 after fetching its fixture commit. `dispatch-gates --ran`: "60 derived famil(ies) accounted for — 59 run, 1 NOT-MEASURED (1 DERIVED from a recorded exit 3)". ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (`packages/spec` untouched.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes (scoped to the touched package). - [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: n/a. - [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 (the 维护者速读 block is the seat's required Chinese summary). - [x] The *Before Submitting* list in CONTRIBUTING.md is done. --- _Generated by [Claude Code](https://claude.ai/code/session_01FEkQ5m1ywz2SChxvNGPb5U)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent efdddc9 commit a0f8654

3 files changed

Lines changed: 75 additions & 8 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/service-datasource': patch
3+
---
4+
5+
fix(service-datasource): an unset env credential is a note after the driver's error, and a failed connect holds no claim on its variable (#28283)
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
Two corrections to the code-defined datasource's env-credential slot (`OS_DATASOURCE_<NAME>_PASSWORD`, #27687):
11+
12+
- **The driver's error explains the failure.** When a code datasource's connect failed with the variable unset, the reason read `<driver error> — datasource 'x' is defined in code and reads its credential from OS_DATASOURCE_X_PASSWORD at connect, which is not set on this host` for every failure, so an unreachable host, a missing driver module or a TLS failure read as a missing password. The sentence is now a trailing note: `<driver error> (note: datasource 'x' is defined in code and reads its credential from OS_DATASOURCE_X_PASSWORD at connect, which is not set on this host)`. A login refusal still names the variable to set. The reason is operator-facing text (the connect verdict, Setup → Datasources, the fail-fast boot message); no code or status changes.
13+
- **A failed connect releases its variable.** The guard that refuses two datasources deriving one variable recorded the owner before the build and released it only on `disconnect`, so a connect that failed kept it and a later datasource deriving the same variable was refused as a collision against one that never connected. The connect that claimed the variable now releases it when it fails. Two datasources that are both live are still refused.

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

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@
1313
* never read it — and a caller-set `origin: 'code'` is not provenance;
1414
* - a declared `credentialsRef` / `external.connector` wins, and a failing
1515
* one stays fail-closed rather than falling back;
16-
* - an unset variable keeps the verdict, and its reason names the variable;
17-
* - two names deriving one variable are refused;
16+
* - an unset variable keeps the verdict, and its reason names the variable —
17+
* as a trailing note after the driver's own error (#28283);
18+
* - two names deriving one variable are refused, and a failed connect holds
19+
* no claim on its variable (#28283);
1820
* - the value reaches no log line and no answer.
1921
* The kernel-level wiring (the admin plugin's switch, the provenance set the
2022
* runtime fills) is pinned in `@objectstack/runtime`'s
@@ -54,9 +56,10 @@ function engine(): ConnectionEngineLike {
5456
/**
5557
* A factory that records the `secret` each build received. `connectFails`
5658
* makes the connect throw the way a database refusing an unauthenticated
57-
* login does.
59+
* login does; `connectError` makes the connect of the named datasources
60+
* throw that error instead.
5861
*/
59-
function recordingFactory(opts: { connectFails?: boolean } = {}) {
62+
function recordingFactory(opts: { connectFails?: boolean; connectError?: { names: string[]; error: () => Error } } = {}) {
6063
const seen: Array<{ name?: string; secret?: string }> = [];
6164
const factory: IDatasourceDriverFactory = {
6265
supports: () => true,
@@ -66,6 +69,7 @@ function recordingFactory(opts: { connectFails?: boolean } = {}) {
6669
return {
6770
driver,
6871
connect: async () => {
72+
if (opts.connectError?.names.includes(spec.name)) throw opts.connectError.error();
6973
if (opts.connectFails) throw new Error('password authentication failed for user "svc"');
7074
},
7175
};
@@ -88,12 +92,13 @@ function service(opts: {
8892
env?: Record<string, string | undefined>;
8993
slot?: boolean;
9094
connectFails?: boolean;
95+
connectError?: { names: string[]; error: () => Error };
9196
secrets?: { resolve?: (ref: string) => Promise<string | undefined> };
9297
} = {}) {
9398
const codeNames = new Set(opts.codeNames ?? ['warehouse']);
9499
const env = opts.env ?? { [datasourceEnvCredentialVar('warehouse')]: VALUE };
95100
const read = vi.fn((variable: string) => env[variable]);
96-
const { factory, seen } = recordingFactory({ connectFails: opts.connectFails });
101+
const { factory, seen } = recordingFactory({ connectFails: opts.connectFails, connectError: opts.connectError });
97102
const { logger, lines } = capturingLogger();
98103
const svc = new DatasourceConnectionService({
99104
factory: () => factory,
@@ -239,6 +244,22 @@ describe('an unset variable keeps the verdict, and the reason names the variable
239244
expect(result.reason).toContain('OS_DATASOURCE_WAREHOUSE_PASSWORD');
240245
});
241246

247+
it("a failure that is not a login refusal leads with the driver's error; the variable is only a trailing note", async () => {
248+
// [#28283] An unreachable host, a missing driver module or a TLS failure
249+
// is not cured by a password: the variable rides along as a note, never as
250+
// the explanation of the failure.
251+
const unreachable = 'connect ECONNREFUSED 127.0.0.1:1';
252+
const { svc } = service({
253+
env: {},
254+
connectError: { names: ['warehouse'], error: () => Object.assign(new Error(unreachable), { code: 'ECONNREFUSED' }) },
255+
});
256+
const result = await svc.connect(warehouse, BOOT).catch((err: Error) => ({ status: 'threw', reason: err.message }));
257+
expect(result.status).toBe('failed-degraded');
258+
expect(result.reason!.startsWith(unreachable)).toBe(true);
259+
expect(result.reason!.slice(unreachable.length)).toMatch(/^ \(note: [^)]*OS_DATASOURCE_WAREHOUSE_PASSWORD[^)]*\)$/);
260+
expect(svc.getConnectionState('warehouse')?.reason).toBe(result.reason);
261+
});
262+
242263
it('a failure on a connect the slot did not apply to keeps its reason unchanged', async () => {
243264
const { svc } = service({ env: {}, connectFails: true });
244265
const result = await svc.connect(warehouse, { context: { origin: 'runtime', trigger: 'runtime-admin' } });
@@ -264,6 +285,23 @@ describe('two datasources that derive one variable are refused', () => {
264285
expect(seen.map((s) => s.name)).toEqual(['eu-sales']);
265286
});
266287

288+
it('a connect that failed holds no claim: a later name deriving the same variable connects', async () => {
289+
// [#28283] The owner is the datasource the variable was read for and
290+
// connected; a failed connect leaves no live pool to collide with.
291+
const { svc, seen } = service({
292+
codeNames: ['eu-sales', 'eu_sales'],
293+
env: { OS_DATASOURCE_EU_SALES_PASSWORD: VALUE },
294+
connectError: { names: ['eu-sales'], error: () => new Error('getaddrinfo ENOTFOUND db.internal') },
295+
});
296+
const first = await svc.connect({ ...warehouse, name: 'eu-sales' }, BOOT);
297+
expect(first.status).toBe('failed-degraded');
298+
const second = await svc
299+
.connect({ ...warehouse, name: 'eu_sales' }, BOOT)
300+
.catch((err: Error) => ({ status: 'threw', reason: err.message }));
301+
expect(second.status).toBe('connected');
302+
expect(seen.map((s) => s.name)).toEqual(['eu-sales', 'eu_sales']);
303+
});
304+
267305
it('a reconnect of the same name is not a collision, and a disconnect releases the variable', async () => {
268306
const { svc, seen } = service({ codeNames: ['eu-sales', 'eu_sales'], env: { OS_DATASOURCE_EU_SALES_PASSWORD: VALUE } });
269307
await svc.connect({ ...warehouse, name: 'eu-sales' }, BOOT);

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -761,7 +761,14 @@ export class DatasourceConnectionService {
761761
// failure above never falls through to here. `envCredentialVar` stays set
762762
// when the slot applied but the variable was empty, so a failed connect
763763
// can name the variable to set.
764+
//
765+
// [#28283] The owner is claimed here, before the build, so the check and
766+
// the claim are one step with no await between them; `claimedVariable`
767+
// records that THIS attempt made the claim, and the failure path below
768+
// releases it. A connect that failed holds no pool, so it must not refuse
769+
// a later name as a collision against it.
764770
let envCredentialVar: string | undefined;
771+
let claimedVariable: string | undefined;
765772
if (!credentialsRef && connector === undefined && !opts.asDefault) {
766773
const variable = this.envCredentialVarFor(name, opts.context);
767774
if (variable !== undefined) {
@@ -777,7 +784,10 @@ export class DatasourceConnectionService {
777784
opts.mappedObjects,
778785
);
779786
}
780-
this.envCredentialOwners.set(variable, name);
787+
if (owner === undefined) {
788+
this.envCredentialOwners.set(variable, name);
789+
claimedVariable = variable;
790+
}
781791
const value = this.cfg.envCredentials?.read(variable);
782792
if (value) secret = value;
783793
else envCredentialVar = variable;
@@ -898,14 +908,20 @@ export class DatasourceConnectionService {
898908
// Before `handleFailure`, which records the unavailable mark: eviction
899909
// deliberately does not touch that map, and the mark must outlive it.
900910
if (registeredByThisAttempt) engine.unregisterDriver?.(registeredByThisAttempt);
911+
if (claimedVariable !== undefined) this.envCredentialOwners.delete(claimedVariable);
901912
// `err` itself is handed on, not just its message: the driver package's
902913
// build output being absent is reported as `err.code ===
903914
// 'ERR_MODULE_NOT_FOUND'`, and that structured signal is gone the moment
904915
// the error is stringified (#5794).
916+
//
917+
// [#28283] The unset variable is a note after the driver's own error,
918+
// never its explanation: an unreachable host, a missing driver module or
919+
// a TLS failure is not cured by a password, and the driver's error is
920+
// the one thing that says which failure this was.
905921
const reason = envCredentialVar === undefined
906922
? errMsg(err)
907-
: `${errMsg(err)} — datasource '${name}' is defined in code and reads its credential from `
908-
+ `${envCredentialVar} at connect, which is not set on this host`;
923+
: `${errMsg(err)} (note: datasource '${name}' is defined in code and reads its credential from `
924+
+ `${envCredentialVar} at connect, which is not set on this host)`;
909925
return this.handleFailure(record, 'failed-degraded', reason, opts.context, opts.objects, opts.mappedObjects, err);
910926
}
911927
}

0 commit comments

Comments
 (0)