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)

.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_<NAME>_PASSWORD`, #27687):
+
+- **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.
+- **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

@@ -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<string, string | undefined>;
   slot?: boolean;
   connectFails?: boolean;
+  connectError?: { names: string[]; error: () => Error };
   secrets?: { resolve?: (ref: string) => Promise<string | undefined> };
 } = {}) {
   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);

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);
     }
   }