fix: surface server detail and cause on connection failures

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+- Thrift HTTP errors (`THTTPException`) now append the server's error detail, e.g. `Received a response with a bad HTTP status code: 403: Invalid access token.`, taken from the server error headers or, failing that, the response body (at most 500 characters). The body is read for at most 64 KiB and 1 second; if either budget runs out, the error reports the status code only and `response` still yields the full body. Kernel-backend errors now carry a non-enumerable `cause` `Error` whose message is `<kernel error code>: <kernel message>` (at most 500 characters; the envelope diagnostics are not included). (#467)
 - CloudFetch retry-exhaustion errors now identify the download and the last failure: the `RetryError` message reads `CloudFetch result download failed: <original message> (last error: <cause>)`. `<cause>` is `HTTP <status>: <statusText>` (or `HTTP <status>` when the status text is empty) for HTTP failures, and `network error <code>`, `network error (<type>)` (e.g. `aborted`, when there is no `code`) or a bare `network error` for network failures (the raw network-error message, which can contain the presigned URL, is not copied). If no cause can be determined, the ` (last error: …)` suffix is omitted. The `RetryError` instance, `errorCode` and `payload` (including the node-fetch `Request` / `Response`) are unchanged. (#471)
 - Report untyped NULL columns as STRING in kernel result metadata, matching Thrift. (#470)
 - Reject non-scalar query parameter values at bind time with a `ParameterError` on both the Thrift and kernel backends, instead of stringifying them (which could leave a Thrift operation stuck before FINISHED). Newly rejected on the Thrift backend: arrays, plain objects, boxed primitives (e.g. `new String('x')`, `new Number(1)`), functions, symbols, and a `DBSQLParameter` that wraps another `DBSQLParameter`. Primitives, `Date`, `Int64`, and `DBSQLParameter` are still accepted.

lib/connection/connections/ThriftHttpConnection.ts

@@ -5,6 +5,7 @@
 */
 
 import { EventEmitter } from 'events';
+import { PassThrough, pipeline } from 'stream';
 import { TBinaryProtocol, TBufferedTransport, Thrift, TProtocol, TProtocolConstructor, TTransport } from 'thrift';
 import fetch, { RequestInit, HeadersInit, Request, Response, FetchError } from 'node-fetch';
 // @ts-expect-error TS7016: Could not find a declaration file for module
@@ -18,16 +19,147 @@ export class THTTPException extends Thrift.TApplicationException {
 
   public readonly response: Response;
 
-  constructor(response: Response) {
-    super(
-      Thrift.TApplicationExceptionType.PROTOCOL_ERROR,
-      `Received a response with a bad HTTP status code: ${response.status}`,
-    );
+  constructor(response: Response, detail?: string) {
+    const message = `Received a response with a bad HTTP status code: ${response.status}`;
+    super(Thrift.TApplicationExceptionType.PROTOCOL_ERROR, detail ? `${message}: ${detail}` : message);
     this.statusCode = response.status;
     this.response = response;
   }
 }
 
+// Server error headers, most specific first.
+const errorDetailHeaders = [
+  'x-thriftserver-error-message',
+  'x-databricks-error-or-redirect-message',
+  'x-databricks-reason-phrase',
+];
+
+const MAX_ERROR_DETAIL_LENGTH = 500;
+const MAX_ERROR_BODY_SCAN = 4096;
+// Budgets for reading an error body; when either runs out the error carries the status only
+const MAX_ERROR_BODY_BYTES = 64 * 1024;
+const MAX_ERROR_BODY_READ_MS = 1000;
+
+function normalizeErrorDetail(value: string): string | undefined {
+  const text = value.slice(0, MAX_ERROR_BODY_SCAN).replace(/\s+/g, ' ').trim();
+  if (text.length === 0) {
+    return undefined;
+  }
+  return text.length > MAX_ERROR_DETAIL_LENGTH ? `${text.slice(0, MAX_ERROR_DETAIL_LENGTH)}…` : text;
+}
+
+function getErrorDetailFromBody(body: Buffer): string | undefined {
+  const text = body.toString('utf8', 0, MAX_ERROR_BODY_SCAN).trim();
+  // Skip HTML pages and binary (e.g. Thrift-encoded) payloads
+  // eslint-disable-next-line no-control-regex
+  if (text.startsWith('<') || /[\u0000-\u0008\u000e-\u001f]/.test(text)) {
+    return undefined;
+  }
+  try {
+    const parsed = JSON.parse(text);
+    if (typeof parsed?.message === 'string') {
+      return normalizeErrorDetail(parsed.message);
+    }
+  } catch {
+    // Not JSON - use the text as is
+  }
+  return normalizeErrorDetail(text);
+}
+
+interface ErrorBodyReadResult {
+  chunks: Buffer[];
+  // `false` when the byte or time budget ran out before the body ended
+  complete: boolean;
+}
+
+// Reads the error body until it ends, it exceeds `MAX_ERROR_BODY_BYTES`, or
+// `MAX_ERROR_BODY_READ_MS` elapses, whichever comes first. The stream is left
+// paused (not destroyed) when a budget runs out, so the rest stays readable.
+function readErrorBody(body: NodeJS.ReadableStream): Promise<ErrorBodyReadResult> {
+  return new Promise((resolve, reject) => {
+    const chunks: Buffer[] = [];
+    let size = 0;
+    let timer: NodeJS.Timeout | undefined;
+
+    const cleanup = () => {
+      clearTimeout(timer);
+      body.removeListener('data', onData);
+      body.removeListener('end', onEnd);
+      body.removeListener('error', onError);
+    };
+    const stop = () => {
+      cleanup();
+      body.pause();
+      resolve({ chunks, complete: false });
+    };
+    function onData(chunk: Buffer | string) {
+      const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk);
+      chunks.push(buffer);
+      size += buffer.length;
+      if (size > MAX_ERROR_BODY_BYTES) {
+        stop();
+      }
+    }
+    function onEnd() {
+      cleanup();
+      resolve({ chunks, complete: true });
+    }
+    function onError(error: unknown) {
+      cleanup();
+      reject(error);
+    }
+
+    timer = setTimeout(stop, MAX_ERROR_BODY_READ_MS);
+    body.on('data', onData);
+    body.once('end', onEnd);
+    body.once('error', onError);
+  });
+}
+
+async function createHTTPException(response: Response): Promise<THTTPException> {
+  for (const header of errorDetailHeaders) {
+    const detail = normalizeErrorDetail(response.headers.get(header) ?? '');
+    if (detail) {
+      return new THTTPException(response, detail);
+    }
+  }
+
+  // Don't start reading a body that is already known to exceed the byte budget
+  const contentLength = Number(response.headers.get('content-length'));
+  if (Number.isFinite(contentLength) && contentLength > MAX_ERROR_BODY_BYTES) {
+    return new THTTPException(response);
+  }
+
+  let result: ErrorBodyReadResult;
+  try {
+    result = await readErrorBody(response.body);
+  } catch {
+    return new THTTPException(response);
+  }
+
+  const init = {
+    url: response.url,
+    status: response.status,
+    statusText: response.statusText,
+    headers: response.headers,
+  };
+  const prefix = Buffer.concat(result.chunks);
+
+  if (!result.complete) {
+    // Budget exhausted: report the status only, and expose a response that still
+    // yields the whole body (the bytes read so far followed by the rest of the stream)
+    const rest = new PassThrough();
+    rest.write(prefix);
+    pipeline(response.body, rest, () => {
+      // Errors reach whoever reads `rest`
+    });
+    return new THTTPException(new Response(rest, init));
+  }
+
+  // The body is consumed now, so expose an equivalent response that can still be read
+  return new THTTPException(new Response(prefix, init), getErrorDetailFromBody(prefix));
+}
+
 // thrift >= 0.25 accepts an optional `maxLength` that @types/thrift doesn't declare.
 type TTransportType = typeof TBufferedTransport & {
   receiver(
@@ -152,7 +284,9 @@ export default class ThriftHttpConnection extends EventEmitter {
       })
       .then(({ response }) => {
         if (response.status !== 200) {
-          throw new THTTPException(response);
+          return createHTTPException(response).then((error) => {
+            throw error;
+          });
         }
         // `responseBuffer` is always set when status is 200, since `makeRequest`
         // assigns it before resolving in that branch and the retry loop only

lib/kernel/KernelErrorMapping.ts

@@ -234,6 +234,12 @@ function buildKernelMetadata(parsed: Record<string, unknown>): KernelMetadata {
   return meta;
 }
 
+const MAX_CAUSE_MESSAGE_LENGTH = 500;
+
+function boundCauseMessage(text: string): string {
+  return text.length > MAX_CAUSE_MESSAGE_LENGTH ? `${text.slice(0, MAX_CAUSE_MESSAGE_LENGTH)}…` : text;
+}
+
 /**
  * Decode a napi-binding error into the typed JS error class.
  *
@@ -315,5 +321,11 @@ export function decodeNapiKernelError(err: unknown): Error {
   if (Object.keys(meta).length > 0) {
     defineErrorMetadata(jsErr, 'kernelMetadata', meta);
   }
+
+  // Attach a bounded summary of the kernel error as `cause` (same descriptor as a
+  // native `new Error(msg, { cause })`). The binding error itself is not used: its
+  // message is the whole envelope, including the unbounded diagnostics, which
+  // `util.inspect` would print for every kernel error.
+  defineErrorMetadata(jsErr, 'cause', new Error(boundCauseMessage(`${code}: ${msg}`)));
   return jsErr;
 }

lib/thrift-backend/ThriftBackend.ts

@@ -130,14 +130,16 @@ export default class ThriftBackend implements IBackend {
         try {
           return await this.openSessionWithKernelBackend(request);
         } catch (kernelError) {
-          // Preserve the Thrift KP001 as the kernel error's cause, but don't clobber a cause
-          // the kernel error may already carry.
-          if (
-            kernelError &&
-            typeof kernelError === 'object' &&
-            (kernelError as { cause?: unknown }).cause === undefined
-          ) {
-            (kernelError as { cause?: unknown }).cause = error;
+          // Preserve the Thrift KP001 at the end of the kernel error's cause chain, so a
+          // cause the kernel error already carries is not clobbered.
+          if (kernelError && typeof kernelError === 'object') {
+            let tail = kernelError as { cause?: unknown };
+            for (let depth = 0; depth < 10 && tail.cause && typeof tail.cause === 'object'; depth += 1) {
+              tail = tail.cause as { cause?: unknown };
+            }
+            if (tail.cause === undefined) {
+              tail.cause = error;
+            }
           }
           logger.log(LogLevel.error, 'Reyden: both Thrift (KP001) and SEA fallback failed');
           throw kernelError;

tests/e2e/connection_failure.test.ts

@@ -0,0 +1,32 @@
+import { expect, AssertionError } from 'chai';
+import { DBSQLClient } from '../../lib';
+import { THTTPException } from '../../lib/connection/connections/ThriftHttpConnection';
+
+import config from './utils/config';
+
+const INVALID_TOKEN = 'dapi-invalid-e2e-token';
+
+// Regression for https://github.com/databricks/databricks-sql-nodejs/issues/467 —
+// a rejected session open should carry the server's detail in `message`.
+describe('Connection failure errors', () => {
+  it('Thrift: includes the server detail in the message of a rejected session open', async () => {
+    const client = new DBSQLClient();
+    try {
+      await client.connect({ host: config.host, path: config.path, token: INVALID_TOKEN });
+      await client.openSession();
+      expect.fail('It should throw an error');
+    } catch (error) {
+      if (error instanceof AssertionError) {
+        throw error;
+      }
+      expect(error).to.be.instanceOf(THTTPException);
+      const { message, statusCode } = error as THTTPException;
+      expect(statusCode).to.be.oneOf([401, 403]);
+      expect(message.startsWith(`Received a response with a bad HTTP status code: ${statusCode}: `)).to.equal(true);
+      expect(message.toLowerCase()).to.include('token');
+      expect(message).to.not.include(INVALID_TOKEN);
+    } finally {
+      await client.close();
+    }
+  });
+});

tests/e2e/kernel/connection-failure-e2e.test.ts

@@ -0,0 +1,47 @@
+import { expect, AssertionError } from 'chai';
+import { DBSQLClient } from '../../../lib';
+import AuthenticationError from '../../../lib/errors/AuthenticationError';
+import { ConnectionOptions } from '../../../lib/contracts/IDBSQLClient';
+import { InternalConnectionOptions } from '../../../lib/contracts/InternalConnectionOptions';
+import { tryGetKernelNative } from '../../../lib/kernel/KernelNativeLoader';
+import config from '../utils/config';
+
+const INVALID_TOKEN = 'dapi-invalid-e2e-token';
+
+// Regression for https://github.com/databricks/databricks-sql-nodejs/issues/467 —
+// a rejected kernel session open should keep the underlying error as `cause`.
+describe('kernel connection failure errors', function suite() {
+  this.timeout(60_000);
+
+  if (tryGetKernelNative() === undefined) {
+    it.skip('kernel native binding not available on this platform');
+    return;
+  }
+
+  it('keeps the server detail in the message and the binding error as cause', async () => {
+    const client = new DBSQLClient();
+    try {
+      await client.connect({
+        host: config.host,
+        path: config.path,
+        token: INVALID_TOKEN,
+        useKernel: true,
+      } as ConnectionOptions & InternalConnectionOptions);
+      await client.openSession();
+      expect.fail('It should throw an error');
+    } catch (error) {
+      if (error instanceof AssertionError) {
+        throw error;
+      }
+      expect(error).to.be.instanceOf(AuthenticationError);
+      const { message, cause } = error as AuthenticationError & { cause?: unknown };
+      expect(message).to.match(/HTTP 40[13]/);
+      expect(message.toLowerCase()).to.include('token');
+      expect(message).to.not.include(INVALID_TOKEN);
+      expect(cause).to.be.instanceOf(Error);
+      expect((cause as Error).message).to.not.include('__databricks_error__');
+    } finally {
+      await client.close();
+    }
+  });
+});