From c625ed761444b71bb815331903b1d97be6a84168 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Wed, 11 Nov 2026 23:04:39 +0000 Subject: [PATCH 1/3] fix: surface server detail and cause on connection failures Thrift THTTPException messages now append the server's error detail (Thrift/Databricks error headers, else a JSON or plain-text body, capped at 500 characters), keeping the existing message prefix. Kernel errors decoded from the napi binding keep the binding error as a non-enumerable `cause`; the Reyden fallback appends the Thrift KP001 rejection at the end of that chain instead of dropping it. Fixes #467 Co-authored-by: Isaac Signed-off-by: Vu Anh Phung --- CHANGELOG.md | 1 + .../connections/ThriftHttpConnection.ts | 72 ++++++++++- lib/kernel/KernelErrorMapping.ts | 12 ++ lib/thrift-backend/ThriftBackend.ts | 18 +-- tests/e2e/connection_failure.test.ts | 32 +++++ .../e2e/kernel/connection-failure-e2e.test.ts | 47 +++++++ .../connections/ThriftHttpConnection.test.ts | 118 +++++++++++++++++- tests/unit/kernel/auth-edge-cases.test.ts | 24 ++++ tests/unit/kernel/error-mapping.test.ts | 39 ++++++ .../ReydenThriftRecoveryOrchestration.test.ts | 21 ++++ 10 files changed, 369 insertions(+), 15 deletions(-) create mode 100644 tests/e2e/connection_failure.test.ts create mode 100644 tests/e2e/kernel/connection-failure-e2e.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f549fa25..c1a5fa47 100644 --- a/CHANGELOG.md +++ b/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). Kernel-backend errors now keep the underlying binding error as `cause`. (#467) - CloudFetch retry-exhaustion errors now identify the download and the last failure: the `RetryError` message reads `CloudFetch result download failed: (last error: )`. `` is `HTTP : ` (or `HTTP ` when the status text is empty) for HTTP failures, and `network error `, `network error ()` (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. diff --git a/lib/connection/connections/ThriftHttpConnection.ts b/lib/connection/connections/ThriftHttpConnection.ts index 2b4f670b..069a9055 100644 --- a/lib/connection/connections/ThriftHttpConnection.ts +++ b/lib/connection/connections/ThriftHttpConnection.ts @@ -18,16 +18,74 @@ 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; + +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); +} + +async function createHTTPException(response: Response): Promise { + for (const header of errorDetailHeaders) { + const detail = normalizeErrorDetail(response.headers.get(header) ?? ''); + if (detail) { + return new THTTPException(response, detail); + } + } + + let body: Buffer; + try { + body = await response.buffer(); + } catch { + return new THTTPException(response); + } + // The body is consumed now, so expose an equivalent response that can still be read + const bufferedResponse = new Response(body, { + url: response.url, + status: response.status, + statusText: response.statusText, + headers: response.headers, + }); + return new THTTPException(bufferedResponse, getErrorDetailFromBody(body)); +} + // thrift >= 0.25 accepts an optional `maxLength` that @types/thrift doesn't declare. type TTransportType = typeof TBufferedTransport & { receiver( @@ -152,7 +210,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 diff --git a/lib/kernel/KernelErrorMapping.ts b/lib/kernel/KernelErrorMapping.ts index 2d151a8c..37d14949 100644 --- a/lib/kernel/KernelErrorMapping.ts +++ b/lib/kernel/KernelErrorMapping.ts @@ -315,5 +315,17 @@ export function decodeNapiKernelError(err: unknown): Error { if (Object.keys(meta).length > 0) { defineErrorMetadata(jsErr, 'kernelMetadata', meta); } + + // Keep the binding error as `cause` (same descriptor as a native + // `new Error(msg, { cause })`), without the internal framing prefix. + try { + err.message = jsonStr; + if (typeof err.stack === 'string') { + err.stack = err.stack.replace(ERROR_SENTINEL, ''); + } + } catch { + // Read-only message/stack: keep the binding error as it is + } + defineErrorMetadata(jsErr, 'cause', err); return jsErr; } diff --git a/lib/thrift-backend/ThriftBackend.ts b/lib/thrift-backend/ThriftBackend.ts index f7b35e0e..0da94562 100644 --- a/lib/thrift-backend/ThriftBackend.ts +++ b/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; diff --git a/tests/e2e/connection_failure.test.ts b/tests/e2e/connection_failure.test.ts new file mode 100644 index 00000000..385cd64f --- /dev/null +++ b/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(); + } + }); +}); diff --git a/tests/e2e/kernel/connection-failure-e2e.test.ts b/tests/e2e/kernel/connection-failure-e2e.test.ts new file mode 100644 index 00000000..12f01ce1 --- /dev/null +++ b/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(); + } + }); +}); diff --git a/tests/unit/connection/connections/ThriftHttpConnection.test.ts b/tests/unit/connection/connections/ThriftHttpConnection.test.ts index d9bffd81..b5924dc1 100644 --- a/tests/unit/connection/connections/ThriftHttpConnection.test.ts +++ b/tests/unit/connection/connections/ThriftHttpConnection.test.ts @@ -2,7 +2,7 @@ import http from 'http'; import { AddressInfo } from 'net'; import { expect } from 'chai'; import thrift, { TBinaryProtocol, TBufferedTransport } from 'thrift'; -import ThriftHttpConnection from '../../../../lib/connection/connections/ThriftHttpConnection'; +import ThriftHttpConnection, { THTTPException } from '../../../../lib/connection/connections/ThriftHttpConnection'; import NullRetryPolicy from '../../../../lib/connection/connections/NullRetryPolicy'; import TCLIService from '../../../../thrift/TCLIService'; import { @@ -22,6 +22,41 @@ type Receiver = ( maxLength?: number, ) => (data: Buffer) => void; +interface Reply { + status: number; + headers?: Record; + body?: string | Buffer; +} + +const PREFIX = 'Received a response with a bad HTTP status code'; + +// Sends one Thrift write to a local server that answers with `reply`, and +// returns the error handed to the pending Thrift callback. +async function failWith(reply: Reply, requestHeaders: Record = {}): Promise { + const server = http.createServer((req, res) => { + req.resume(); + res.writeHead(reply.status, reply.headers); + res.end(reply.body); + }); + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve); + }); + try { + const { port } = server.address() as AddressInfo; + const connection = new ThriftHttpConnection( + { url: `http://127.0.0.1:${port}/`, getRetryPolicy: async () => new NullRetryPolicy() }, + { headers: requestHeaders }, + ); + return await new Promise((resolve) => { + connection.client = { _reqs: { 1: (error: unknown) => resolve(error as THTTPException) } } as any; + connection.write(Buffer.from('x'), 1); + }); + } finally { + server.closeAllConnections(); + server.close(); + } +} + describe('ThriftHttpConnection', () => { let server: http.Server; let url: string; @@ -105,3 +140,84 @@ describe('ThriftHttpConnection', () => { expect(maxLengths).to.deep.equal([requestLengths[0], replyLengths[0]]); }); }); + +describe('ThriftHttpConnection', () => { + describe('non-200 responses', () => { + it('keeps the bare message when the server sends no detail', async () => { + const error = await failWith({ status: 401 }); + expect(error).to.be.instanceOf(THTTPException); + expect(error.message).to.equal(`${PREFIX}: 401`); + expect(error.statusCode).to.equal(401); + }); + + it('appends the Databricks reason-phrase header without consuming the body', async () => { + const body = '{"error_code":403,"message":"Invalid access token. [ReqId: abc]"}'; + const error = await failWith({ + status: 403, + headers: { 'x-databricks-reason-phrase': 'Invalid access token.', 'content-type': 'application/json' }, + body, + }); + expect(error.message).to.equal(`${PREFIX}: 403: Invalid access token.`); + expect(error.statusCode).to.equal(403); + expect(await error.response.text()).to.equal(body); + }); + + it('prefers the Thrift server error header over other detail', async () => { + const error = await failWith({ + status: 404, + headers: { + 'x-thriftserver-error-message': 'ENDPOINT_NOT_FOUND: SQL warehouse does not exist', + 'x-databricks-reason-phrase': 'Not Found', + 'content-type': 'application/x-thrift', + }, + body: Buffer.from([0x80, 0x01, 0x00, 0x03, 0x00, 0x00, 0x00, 0x05]), + }); + expect(error.message).to.equal(`${PREFIX}: 404: ENDPOINT_NOT_FOUND: SQL warehouse does not exist`); + }); + + it('falls back to the JSON body message and keeps the body readable', async () => { + const body = '{"error_code":401,"message":"Invalid access token. [ReqId: abc]"}'; + const error = await failWith({ status: 401, headers: { 'content-type': 'application/json' }, body }); + expect(error.message).to.equal(`${PREFIX}: 401: Invalid access token. [ReqId: abc]`); + expect(error.response.status).to.equal(401); + expect(error.response.headers.get('content-type')).to.equal('application/json'); + expect(await error.response.text()).to.equal(body); + }); + + it('falls back to a plain-text body', async () => { + const error = await failWith({ + status: 401, + headers: { 'content-type': 'application/x-thrift' }, + body: 'Authentication Error [INVALID_AUTHORIZATION_SPECIFICATION]: Invalid access token\r\n', + }); + expect(error.message).to.equal( + `${PREFIX}: 401: Authentication Error [INVALID_AUTHORIZATION_SPECIFICATION]: Invalid access token`, + ); + }); + + it('ignores HTML and binary bodies', async () => { + const html = await failWith({ + status: 502, + headers: { 'content-type': 'text/html' }, + body: 'Bad Gateway', + }); + expect(html.message).to.equal(`${PREFIX}: 502`); + + const binary = await failWith({ status: 500, body: Buffer.from([0x80, 0x01, 0x00, 0x03, 0x41, 0x00]) }); + expect(binary.message).to.equal(`${PREFIX}: 500`); + }); + + it('collapses whitespace and bounds the detail length', async () => { + const error = await failWith({ status: 401, body: `first\n\tline ${'x'.repeat(5000)}` }); + const detail = error.message.slice(`${PREFIX}: 401: `.length); + expect(detail.startsWith('first line xxx')).to.equal(true); + expect(detail.length).to.equal(501); + expect(detail.endsWith('…')).to.equal(true); + }); + + it('never echoes request headers', async () => { + const error = await failWith({ status: 401 }, { Authorization: 'Bearer dapi-secret-token' }); + expect(error.message).to.not.include('dapi-secret-token'); + }); + }); +}); diff --git a/tests/unit/kernel/auth-edge-cases.test.ts b/tests/unit/kernel/auth-edge-cases.test.ts index 6c837fce..ca23e8ba 100644 --- a/tests/unit/kernel/auth-edge-cases.test.ts +++ b/tests/unit/kernel/auth-edge-cases.test.ts @@ -292,6 +292,30 @@ describe('KernelBackend — kernel error envelope decoding (DA-F1)', () => { expect((caught as Error).message).to.match(/invalid_client/); }); + it('preserves the napi error as the AuthenticationError cause on session-open rejection', async () => { + const envelopeJson = '{"code":"Unauthenticated","message":"HTTP 401: Invalid access token","httpStatus":401}'; + const binding = bindingRejectingWith(envelopeJson); + const backend = new KernelBackend({ nativeBinding: binding, context: makeFakeContext() }); + await backend.connect(validConnectArgs); + + let caught: unknown; + try { + await backend.openSession({}); + } catch (e) { + caught = e; + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const err = caught as any; + expect(err).to.be.instanceOf(AuthenticationError); + expect(err.name).to.equal('AuthenticationError'); + expect(err.message).to.equal('HTTP 401: Invalid access token'); + expect(err.kernelMetadata?.httpStatus).to.equal(401); + expect(err.cause).to.be.instanceOf(Error); + expect(err.cause.message).to.equal(envelopeJson); + expect(String(err.cause.stack)).to.not.contain('__databricks_error__'); + expect(Object.keys(err)).to.not.include('cause'); + }); + it('maps NetworkError kernel envelope → HiveDriverError with kernel message preserved', async () => { const binding = bindingRejectingWith( '{"code":"NetworkError","message":"OIDC discovery failed: connection refused"}', diff --git a/tests/unit/kernel/error-mapping.test.ts b/tests/unit/kernel/error-mapping.test.ts index 0dad3335..9af18488 100644 --- a/tests/unit/kernel/error-mapping.test.ts +++ b/tests/unit/kernel/error-mapping.test.ts @@ -312,3 +312,42 @@ describe('KernelErrorMapping.decodeNapiKernelError — diagnostics enrichment (T expect(err.kernelMetadata?.diagnosticInfo).to.equal('ArithmeticException'); // Thrift: response.errorMessage }); }); + +describe('KernelErrorMapping.decodeNapiKernelError — cause chain', () => { + const SENTINEL = '__databricks_error__:'; + + const cases: Array<[string, Function]> = [ + ['Unauthenticated', AuthenticationError], + ['PermissionDenied', AuthenticationError], + ['SqlError', OperationStateError], + ['Cancelled', OperationStateError], + ['InvalidArgument', ParameterError], + ['NetworkError', HiveDriverError], + ['SomeFutureVariant', HiveDriverError], + ]; + + cases.forEach(([code, expectedClass]) => { + it(`keeps the binding error as a non-enumerable cause for ${code}`, () => { + const json = JSON.stringify({ code, message: 'HTTP 403: Invalid access token', httpStatus: 403 }); + const napiError = new Error(SENTINEL + json); + + const err = decodeNapiKernelError(napiError) as Error & { cause?: unknown }; + + expect(err).to.be.instanceOf(expectedClass); + expect(err.message).to.equal('HTTP 403: Invalid access token'); + expect(err.cause).to.equal(napiError); + expect(napiError.message).to.equal(json); + expect(String(napiError.stack)).to.not.contain(SENTINEL); + const descriptor = Object.getOwnPropertyDescriptor(err, 'cause'); + expect(descriptor?.enumerable).to.equal(false); + expect(descriptor?.writable).to.equal(true); + }); + }); + + it('adds no cause to binding-side errors that are returned as-is', () => { + const plain = new Error('openSession: `token` is required'); + const err = decodeNapiKernelError(plain) as Error & { cause?: unknown }; + expect(err).to.equal(plain); + expect(err.cause).to.equal(undefined); + }); +}); diff --git a/tests/unit/thrift-backend/ReydenThriftRecoveryOrchestration.test.ts b/tests/unit/thrift-backend/ReydenThriftRecoveryOrchestration.test.ts index 388d49b9..36cf8aab 100644 --- a/tests/unit/thrift-backend/ReydenThriftRecoveryOrchestration.test.ts +++ b/tests/unit/thrift-backend/ReydenThriftRecoveryOrchestration.test.ts @@ -126,6 +126,27 @@ describe('Reyden Thrift Auto-Recovery — Orchestration', () => { expect(thrown.cause).to.equal(thriftError); }); + it('appends the Thrift rejection after a cause the kernel error already carries', async () => { + const backend = makeBackend(); + const thriftError = kp001Error(); + const napiError = new Error('{"code":"Unauthenticated","message":"denied"}'); + const kernelError = new Error('denied'); + Object.defineProperty(kernelError, 'cause', { value: napiError, writable: true, configurable: true }); + sandbox.stub(backend as any, 'openSessionWithThrift').rejects(thriftError); + sandbox.stub(backend as any, 'openSessionWithKernelBackend').rejects(kernelError); + + let thrown: any; + try { + await backend.openSession({} as any); + } catch (e) { + thrown = e; + } + + expect(thrown).to.equal(kernelError); + expect(thrown.cause).to.equal(napiError); + expect(thrown.cause.cause).to.equal(thriftError); + }); + it('reuses one fallback KernelBackend across sessions and closes it on close()', async () => { const backend = makeBackend(); const fakeKernel = { From 454ea2155e57d787f326a3704fb61d093deb1736 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 12 Nov 2026 08:35:41 +0000 Subject: [PATCH 2/3] engineer-bot: address @mention on PR #587 Signed-off-by: peco-engineer-bot[bot] --- CHANGELOG.md | 2 +- .../connections/ThriftHttpConnection.ts | 86 +++++++++++++++++-- .../connections/ThriftHttpConnection.test.ts | 67 ++++++++++++++- 3 files changed, 145 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1a5fa47..9509256a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +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). Kernel-backend errors now keep the underlying binding error as `cause`. (#467) +- 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 keep the underlying binding error as `cause`. (#467) - CloudFetch retry-exhaustion errors now identify the download and the last failure: the `RetryError` message reads `CloudFetch result download failed: (last error: )`. `` is `HTTP : ` (or `HTTP ` when the status text is empty) for HTTP failures, and `network error `, `network error ()` (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. diff --git a/lib/connection/connections/ThriftHttpConnection.ts b/lib/connection/connections/ThriftHttpConnection.ts index 069a9055..ca3c9541 100644 --- a/lib/connection/connections/ThriftHttpConnection.ts +++ b/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 @@ -35,6 +36,9 @@ const errorDetailHeaders = [ 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(); @@ -62,6 +66,56 @@ function getErrorDetailFromBody(body: Buffer): string | undefined { 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 { + 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 { for (const header of errorDetailHeaders) { const detail = normalizeErrorDetail(response.headers.get(header) ?? ''); @@ -70,20 +124,40 @@ async function createHTTPException(response: Response): Promise } } - let body: Buffer; + // 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 { - body = await response.buffer(); + result = await readErrorBody(response.body); } catch { return new THTTPException(response); } - // The body is consumed now, so expose an equivalent response that can still be read - const bufferedResponse = new Response(body, { + + const init = { url: response.url, status: response.status, statusText: response.statusText, headers: response.headers, - }); - return new THTTPException(bufferedResponse, getErrorDetailFromBody(body)); + }; + 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. diff --git a/tests/unit/connection/connections/ThriftHttpConnection.test.ts b/tests/unit/connection/connections/ThriftHttpConnection.test.ts index b5924dc1..a4d09928 100644 --- a/tests/unit/connection/connections/ThriftHttpConnection.test.ts +++ b/tests/unit/connection/connections/ThriftHttpConnection.test.ts @@ -32,9 +32,18 @@ const PREFIX = 'Received a response with a bad HTTP status code'; // Sends one Thrift write to a local server that answers with `reply`, and // returns the error handed to the pending Thrift callback. -async function failWith(reply: Reply, requestHeaders: Record = {}): Promise { +async function failWith( + reply: Reply | ((res: http.ServerResponse) => void), + requestHeaders: Record = {}, + // Runs before the server is shut down, e.g. to read a body that is still streaming + whileConnected?: (error: THTTPException) => Promise, +): Promise { const server = http.createServer((req, res) => { req.resume(); + if (typeof reply === 'function') { + reply(res); + return; + } res.writeHead(reply.status, reply.headers); res.end(reply.body); }); @@ -47,10 +56,12 @@ async function failWith(reply: Reply, requestHeaders: Record = { { url: `http://127.0.0.1:${port}/`, getRetryPolicy: async () => new NullRetryPolicy() }, { headers: requestHeaders }, ); - return await new Promise((resolve) => { - connection.client = { _reqs: { 1: (error: unknown) => resolve(error as THTTPException) } } as any; + const error = await new Promise((resolve) => { + connection.client = { _reqs: { 1: (err: unknown) => resolve(err as THTTPException) } } as any; connection.write(Buffer.from('x'), 1); }); + await whileConnected?.(error); + return error; } finally { server.closeAllConnections(); server.close(); @@ -215,6 +226,56 @@ describe('ThriftHttpConnection', () => { expect(detail.endsWith('…')).to.equal(true); }); + it('reports the status only when a chunked body exceeds the byte budget, keeping the body readable', async () => { + const chunk = `${'y'.repeat(1023)}\n`; + const chunkCount = 200; // 200 KiB, beyond the 64 KiB budget + let body: string | undefined; + const error = await failWith( + (res) => { + res.writeHead(401, { 'content-type': 'text/plain' }); + for (let i = 0; i < chunkCount; i += 1) { + res.write(chunk); + } + res.end(); + }, + {}, + async (err) => { + body = await err.response.text(); + }, + ); + expect(error.message).to.equal(`${PREFIX}: 401`); + expect(error.statusCode).to.equal(401); + expect(error.response.status).to.equal(401); + expect(error.response.headers.get('content-type')).to.equal('text/plain'); + expect(body).to.equal(chunk.repeat(chunkCount)); + }); + + it('does not read a body whose Content-Length exceeds the byte budget', async () => { + const body = 'z'.repeat(100 * 1024); + let received: string | undefined; + const error = await failWith( + { status: 401, headers: { 'content-length': `${body.length}` }, body }, + {}, + async (err) => { + received = await err.response.text(); + }, + ); + expect(error.message).to.equal(`${PREFIX}: 401`); + expect(received).to.equal(body); + }); + + it('reports the status only when the body does not end within the time budget', async function test() { + this.timeout(10_000); + const started = Date.now(); + const error = await failWith((res) => { + res.writeHead(503, { 'content-type': 'text/plain' }); + res.write('Service temporarily unavailable'); // never ended + }); + expect(error.message).to.equal(`${PREFIX}: 503`); + expect(error.statusCode).to.equal(503); + expect(Date.now() - started).to.be.below(5_000); + }); + it('never echoes request headers', async () => { const error = await failWith({ status: 401 }, { Authorization: 'Bearer dapi-secret-token' }); expect(error.message).to.not.include('dapi-secret-token'); From cc1e2137a9f09c33f092ba7e65803952e7009cd7 Mon Sep 17 00:00:00 2001 From: "peco-engineer-bot[bot]" Date: Thu, 12 Nov 2026 08:45:40 +0000 Subject: [PATCH 3/3] engineer-bot: address @mention on PR #587 Signed-off-by: peco-engineer-bot[bot] --- CHANGELOG.md | 2 +- lib/kernel/KernelErrorMapping.ts | 22 ++++++++--------- tests/unit/kernel/auth-edge-cases.test.ts | 4 ++-- tests/unit/kernel/error-mapping.test.ts | 29 +++++++++++++++++++---- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbd2a77d..7fa36788 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +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 keep the underlying binding error as `cause`. (#467) +- 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 `: ` (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: (last error: )`. `` is `HTTP : ` (or `HTTP ` when the status text is empty) for HTTP failures, and `network error `, `network error ()` (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. diff --git a/lib/kernel/KernelErrorMapping.ts b/lib/kernel/KernelErrorMapping.ts index 37d14949..9c18c00b 100644 --- a/lib/kernel/KernelErrorMapping.ts +++ b/lib/kernel/KernelErrorMapping.ts @@ -234,6 +234,12 @@ function buildKernelMetadata(parsed: Record): 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. * @@ -316,16 +322,10 @@ export function decodeNapiKernelError(err: unknown): Error { defineErrorMetadata(jsErr, 'kernelMetadata', meta); } - // Keep the binding error as `cause` (same descriptor as a native - // `new Error(msg, { cause })`), without the internal framing prefix. - try { - err.message = jsonStr; - if (typeof err.stack === 'string') { - err.stack = err.stack.replace(ERROR_SENTINEL, ''); - } - } catch { - // Read-only message/stack: keep the binding error as it is - } - defineErrorMetadata(jsErr, 'cause', err); + // 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; } diff --git a/tests/unit/kernel/auth-edge-cases.test.ts b/tests/unit/kernel/auth-edge-cases.test.ts index ca23e8ba..fe606461 100644 --- a/tests/unit/kernel/auth-edge-cases.test.ts +++ b/tests/unit/kernel/auth-edge-cases.test.ts @@ -292,7 +292,7 @@ describe('KernelBackend — kernel error envelope decoding (DA-F1)', () => { expect((caught as Error).message).to.match(/invalid_client/); }); - it('preserves the napi error as the AuthenticationError cause on session-open rejection', async () => { + it('attaches the kernel code and message as the AuthenticationError cause on session-open rejection', async () => { const envelopeJson = '{"code":"Unauthenticated","message":"HTTP 401: Invalid access token","httpStatus":401}'; const binding = bindingRejectingWith(envelopeJson); const backend = new KernelBackend({ nativeBinding: binding, context: makeFakeContext() }); @@ -311,7 +311,7 @@ describe('KernelBackend — kernel error envelope decoding (DA-F1)', () => { expect(err.message).to.equal('HTTP 401: Invalid access token'); expect(err.kernelMetadata?.httpStatus).to.equal(401); expect(err.cause).to.be.instanceOf(Error); - expect(err.cause.message).to.equal(envelopeJson); + expect(err.cause.message).to.equal('Unauthenticated: HTTP 401: Invalid access token'); expect(String(err.cause.stack)).to.not.contain('__databricks_error__'); expect(Object.keys(err)).to.not.include('cause'); }); diff --git a/tests/unit/kernel/error-mapping.test.ts b/tests/unit/kernel/error-mapping.test.ts index 9af18488..51454df9 100644 --- a/tests/unit/kernel/error-mapping.test.ts +++ b/tests/unit/kernel/error-mapping.test.ts @@ -327,7 +327,7 @@ describe('KernelErrorMapping.decodeNapiKernelError — cause chain', () => { ]; cases.forEach(([code, expectedClass]) => { - it(`keeps the binding error as a non-enumerable cause for ${code}`, () => { + it(`attaches a non-enumerable code and message cause for ${code}`, () => { const json = JSON.stringify({ code, message: 'HTTP 403: Invalid access token', httpStatus: 403 }); const napiError = new Error(SENTINEL + json); @@ -335,15 +335,36 @@ describe('KernelErrorMapping.decodeNapiKernelError — cause chain', () => { expect(err).to.be.instanceOf(expectedClass); expect(err.message).to.equal('HTTP 403: Invalid access token'); - expect(err.cause).to.equal(napiError); - expect(napiError.message).to.equal(json); - expect(String(napiError.stack)).to.not.contain(SENTINEL); + expect(err.cause).to.be.instanceOf(Error); + expect(err.cause).to.not.equal(napiError); + expect((err.cause as Error).message).to.equal(`${code}: HTTP 403: Invalid access token`); + expect(napiError.message).to.equal(SENTINEL + json); const descriptor = Object.getOwnPropertyDescriptor(err, 'cause'); expect(descriptor?.enumerable).to.equal(false); expect(descriptor?.writable).to.equal(true); }); }); + it('keeps the envelope diagnostics out of the cause and bounds its length', () => { + const json = JSON.stringify({ + code: 'SqlError', + message: `boom ${'m'.repeat(2000)}`, + diagnosticInfo: 'secret-diagnostic-info', + errorDetailsJson: `{"details":"${'d'.repeat(10000)}"}`, + }); + + const err = decodeNapiKernelError(new Error(SENTINEL + json)) as ErrorWithSqlState & { cause?: Error }; + + expect(err.kernelMetadata?.diagnosticInfo).to.equal('secret-diagnostic-info'); + const causeMessage = String(err.cause?.message); + expect(causeMessage.startsWith('SqlError: boom mmm')).to.equal(true); + expect(causeMessage.length).to.equal(501); + expect(causeMessage.endsWith('…')).to.equal(true); + expect(causeMessage).to.not.contain('secret-diagnostic-info'); + expect(causeMessage).to.not.contain('details'); + expect(causeMessage).to.not.contain(SENTINEL); + }); + it('adds no cause to binding-side errors that are returned as-is', () => { const plain = new Error('openSession: `token` is required'); const err = decodeNapiKernelError(plain) as Error & { cause?: unknown };