From d3ceca42a2a5e84a77b3329e5a0ee6e800329d4d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 04:22:15 +0000 Subject: [PATCH] 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 Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FEkQ5m1ywz2SChxvNGPb5U --- .../28283-env-credential-note-and-claim.md | 13 +++++ .../datasource-env-credential.test.ts | 48 +++++++++++++++++-- .../src/datasource-connection-service.ts | 22 +++++++-- 3 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 .changeset/28283-env-credential-note-and-claim.md diff --git a/.changeset/28283-env-credential-note-and-claim.md b/.changeset/28283-env-credential-note-and-claim.md new file mode 100644 index 00000000000..2941b915f4b --- /dev/null +++ b/.changeset/28283-env-credential-note-and-claim.md @@ -0,0 +1,13 @@ +--- +'@objectstack/service-datasource': patch +--- + +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) + +Clause-②: no +Metadata-protocol: none + +Two corrections to the code-defined datasource's env-credential slot (`OS_DATASOURCE__PASSWORD`, #27687): + +- **The driver's error explains the failure.** When a code datasource's connect failed with the variable unset, the reason read ` — 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: ` (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. +- **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. diff --git a/packages/services/service-datasource/src/__tests__/datasource-env-credential.test.ts b/packages/services/service-datasource/src/__tests__/datasource-env-credential.test.ts index ba60b89b663..3d575864199 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-env-credential.test.ts +++ b/packages/services/service-datasource/src/__tests__/datasource-env-credential.test.ts @@ -13,8 +13,10 @@ * never read it — and a caller-set `origin: 'code'` is not provenance; * - a declared `credentialsRef` / `external.connector` wins, and a failing * one stays fail-closed rather than falling back; - * - an unset variable keeps the verdict, and its reason names the variable; - * - two names deriving one variable are refused; + * - an unset variable keeps the verdict, and its reason names the variable — + * as a trailing note after the driver's own error (#28283); + * - two names deriving one variable are refused, and a failed connect holds + * no claim on its variable (#28283); * - the value reaches no log line and no answer. * The kernel-level wiring (the admin plugin's switch, the provenance set the * runtime fills) is pinned in `@objectstack/runtime`'s @@ -54,9 +56,10 @@ function engine(): ConnectionEngineLike { /** * A factory that records the `secret` each build received. `connectFails` * makes the connect throw the way a database refusing an unauthenticated - * login does. + * login does; `connectError` makes the connect of the named datasources + * throw that error instead. */ -function recordingFactory(opts: { connectFails?: boolean } = {}) { +function recordingFactory(opts: { connectFails?: boolean; connectError?: { names: string[]; error: () => Error } } = {}) { const seen: Array<{ name?: string; secret?: string }> = []; const factory: IDatasourceDriverFactory = { supports: () => true, @@ -66,6 +69,7 @@ function recordingFactory(opts: { connectFails?: boolean } = {}) { return { driver, connect: async () => { + if (opts.connectError?.names.includes(spec.name)) throw opts.connectError.error(); if (opts.connectFails) throw new Error('password authentication failed for user "svc"'); }, }; @@ -88,12 +92,13 @@ function service(opts: { env?: Record; slot?: boolean; connectFails?: boolean; + connectError?: { names: string[]; error: () => Error }; secrets?: { resolve?: (ref: string) => Promise }; } = {}) { const codeNames = new Set(opts.codeNames ?? ['warehouse']); const env = opts.env ?? { [datasourceEnvCredentialVar('warehouse')]: VALUE }; const read = vi.fn((variable: string) => env[variable]); - const { factory, seen } = recordingFactory({ connectFails: opts.connectFails }); + const { factory, seen } = recordingFactory({ connectFails: opts.connectFails, connectError: opts.connectError }); const { logger, lines } = capturingLogger(); const svc = new DatasourceConnectionService({ factory: () => factory, @@ -239,6 +244,22 @@ describe('an unset variable keeps the verdict, and the reason names the variable expect(result.reason).toContain('OS_DATASOURCE_WAREHOUSE_PASSWORD'); }); + it("a failure that is not a login refusal leads with the driver's error; the variable is only a trailing note", async () => { + // [#28283] An unreachable host, a missing driver module or a TLS failure + // is not cured by a password: the variable rides along as a note, never as + // the explanation of the failure. + const unreachable = 'connect ECONNREFUSED 127.0.0.1:1'; + const { svc } = service({ + env: {}, + connectError: { names: ['warehouse'], error: () => Object.assign(new Error(unreachable), { code: 'ECONNREFUSED' }) }, + }); + const result = await svc.connect(warehouse, BOOT).catch((err: Error) => ({ status: 'threw', reason: err.message })); + expect(result.status).toBe('failed-degraded'); + expect(result.reason!.startsWith(unreachable)).toBe(true); + expect(result.reason!.slice(unreachable.length)).toMatch(/^ \(note: [^)]*OS_DATASOURCE_WAREHOUSE_PASSWORD[^)]*\)$/); + expect(svc.getConnectionState('warehouse')?.reason).toBe(result.reason); + }); + it('a failure on a connect the slot did not apply to keeps its reason unchanged', async () => { const { svc } = service({ env: {}, connectFails: true }); 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', () => { expect(seen.map((s) => s.name)).toEqual(['eu-sales']); }); + it('a connect that failed holds no claim: a later name deriving the same variable connects', async () => { + // [#28283] The owner is the datasource the variable was read for and + // connected; a failed connect leaves no live pool to collide with. + const { svc, seen } = service({ + codeNames: ['eu-sales', 'eu_sales'], + env: { OS_DATASOURCE_EU_SALES_PASSWORD: VALUE }, + connectError: { names: ['eu-sales'], error: () => new Error('getaddrinfo ENOTFOUND db.internal') }, + }); + const first = await svc.connect({ ...warehouse, name: 'eu-sales' }, BOOT); + expect(first.status).toBe('failed-degraded'); + const second = await svc + .connect({ ...warehouse, name: 'eu_sales' }, BOOT) + .catch((err: Error) => ({ status: 'threw', reason: err.message })); + expect(second.status).toBe('connected'); + expect(seen.map((s) => s.name)).toEqual(['eu-sales', 'eu_sales']); + }); + it('a reconnect of the same name is not a collision, and a disconnect releases the variable', async () => { const { svc, seen } = service({ codeNames: ['eu-sales', 'eu_sales'], env: { OS_DATASOURCE_EU_SALES_PASSWORD: VALUE } }); await svc.connect({ ...warehouse, name: 'eu-sales' }, BOOT); diff --git a/packages/services/service-datasource/src/datasource-connection-service.ts b/packages/services/service-datasource/src/datasource-connection-service.ts index 5f00dac592d..2753417323b 100644 --- a/packages/services/service-datasource/src/datasource-connection-service.ts +++ b/packages/services/service-datasource/src/datasource-connection-service.ts @@ -761,7 +761,14 @@ export class DatasourceConnectionService { // failure above never falls through to here. `envCredentialVar` stays set // when the slot applied but the variable was empty, so a failed connect // can name the variable to set. + // + // [#28283] The owner is claimed here, before the build, so the check and + // the claim are one step with no await between them; `claimedVariable` + // records that THIS attempt made the claim, and the failure path below + // releases it. A connect that failed holds no pool, so it must not refuse + // a later name as a collision against it. let envCredentialVar: string | undefined; + let claimedVariable: string | undefined; if (!credentialsRef && connector === undefined && !opts.asDefault) { const variable = this.envCredentialVarFor(name, opts.context); if (variable !== undefined) { @@ -777,7 +784,10 @@ export class DatasourceConnectionService { opts.mappedObjects, ); } - this.envCredentialOwners.set(variable, name); + if (owner === undefined) { + this.envCredentialOwners.set(variable, name); + claimedVariable = variable; + } const value = this.cfg.envCredentials?.read(variable); if (value) secret = value; else envCredentialVar = variable; @@ -898,14 +908,20 @@ export class DatasourceConnectionService { // 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); + if (claimedVariable !== undefined) this.envCredentialOwners.delete(claimedVariable); // `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 // the error is stringified (#5794). + // + // [#28283] The unset variable is a note after the driver's own error, + // never its explanation: an unreachable host, a missing driver module or + // a TLS failure is not cured by a password, and the driver's error is + // the one thing that says which failure this was. const reason = envCredentialVar === undefined ? errMsg(err) - : `${errMsg(err)} — datasource '${name}' is defined in code and reads its credential from ` - + `${envCredentialVar} at connect, which is not set on this host`; + : `${errMsg(err)} (note: datasource '${name}' is defined in code and reads its credential from ` + + `${envCredentialVar} at connect, which is not set on this host)`; return this.handleFailure(record, 'failed-degraded', reason, opts.context, opts.objects, opts.mappedObjects, err); } }