[vuanhphung]

When the server rejects a session open, the Thrift backend threw THTTPException with only Received a response with a bad HTTP status code: 403, dropping the detail the server sent (x-databricks-reason-phrase: Invalid access token. plus a JSON body). The kernel backend's AuthenticationError already carried the detail but had no cause, so the underlying binding error was lost.

THTTPException now appends the server detail after the unchanged prefix, e.g. Received a response with a bad HTTP status code: 403: Invalid access token.. The detail comes from x-thriftserver-error-message, x-databricks-error-or-redirect-message or x-databricks-reason-phrase. When none of those headers is present, it falls back to the response body (JSON message, else plain text; HTML and binary bodies are skipped), capped at 500 characters. When the body is read, err.response is replaced with an equivalent buffered response, so it stays readable. The 200 path is unchanged. Kernel errors decoded from the napi binding now keep the binding error as a non-enumerable cause (same descriptor as native { cause }, internal __databricks_error__: prefix stripped). The Reyden KP001 fallback now appends the Thrift rejection to the end of that cause chain instead of dropping it. Error classes, statusCode, sqlState and kernelMetadata are unchanged.

Verification: new unit tests drive ThriftHttpConnection against a local HTTP server (header precedence, JSON/text body fallback, HTML/binary skipping, length cap, body still readable) and cover the kernel cause for every mapped class plus the Reyden chain. npm test, lint, type-check and build pass. Live against a warehouse, using an invalid token and a nonexistent warehouse, both backends now surface the server detail (Thrift 404: ENDPOINT_NOT_FOUND: SQL warehouse … does not exist), and the kernel errors carry cause. The new e2e specs (tests/e2e/connection_failure.test.ts, tests/e2e/kernel/connection-failure-e2e.test.ts) pass on this branch and fail on main. No perf impact: the success path is byte-for-byte unchanged and the new work only runs after the server has already returned an error.

Not addressed here: the coverage test also expects a cause on the Thrift error. On Thrift, THTTPException is itself the transport error (it carries statusCode and response), so there is no underlying error to preserve, and adding a synthetic one would only duplicate the message.

Fixes #467

This pull request and its description were written by Isaac.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Low

Looks good, with 1 low concern: the error body is read in full even though only the first 4 KiB is inspected. The header-first detail extraction, the 200-path invariance, the kernel cause descriptor and the Reyden cause-chain append are all correct, and each has targeted unit and e2e tests. Nit: putting the Thrift KP001 at the tail of the chain makes it the cause of the napi binding error, which reads a little oddly, but it keeps the existing cause intact as intended.

[peco-review-bot]

🔵 Low — On the header-less error path, response.buffer() reads the whole error body into memory, but only the first 4 KiB (MAX_ERROR_BODY_SCAN) is ever scanned for detail. Before this change, non-200 bodies were never read. Now a large error page from a proxy or gateway, or a slow body stream, is fully buffered (and awaited) before the caller gets the exception. That is new work on a path that used to fail fast.

The buffered copy is kept so that err.response stays readable, which is a deliberate trade-off. Consider reading at most a bounded prefix, for example by streaming response.body until MAX_ERROR_BODY_SCAN bytes. That would mean the exposed response holds only a truncated body, so it is a design choice for the author. At minimum, note the unbounded read in the code comment.

[vuanhphung]

@bot Could we bound this diagnostic body read by both bytes and time, falling back to the status-only error when either budget is exhausted? MAX_ERROR_BODY_SCAN only limits parsing; response.buffer() still consumes the complete body and delays reporting an already-known HTTP failure.

[peco-engineer-bot]

Pushed 454ea21 to fix/issue-467-connection-failure-detail addressing your request.

Started from c625ed7. Replaced the full response.buffer() read in ThriftHttpConnection.ts. The body is now read for at most 64 KiB and 1 s. A Content-Length over 64 KiB skips the read. When a budget runs out, the error carries the status only, and error.response still returns the full body: the bytes already read, then the rest of the stream. Added 3 tests: chunked body over the byte limit, oversized Content-Length, and a body that never ends. Updated CHANGELOG. npm test (1555 passing), type-check and prettier pass. npm run lint did not run: ESLint can't find the airbnb-base config here. E2E tests not run.

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low concern: a JSON error body longer than 4 KiB loses its message field and the error shows a raw JSON prefix instead. The other changes behave as described and the tests cover them: the bounded body read keeps response readable, the kernel cause uses a non-enumerable descriptor, and the Reyden chain appends at the end. Follow-up, not caused by this PR: the 10-level depth cap in the Reyden cause walk still drops the Thrift error when a cause chain is cyclic or deeper than 10 levels. Before this PR, any existing cause made the code drop it, so this is no worse.

[peco-review-bot]

🔵 Low — getErrorDetailFromBody only decodes the first MAX_ERROR_BODY_SCAN (4096) bytes before calling JSON.parse, even though the whole body (up to 64 KiB) is already in prefix. When a JSON error body is longer than 4 KiB (for example, one with a long details array or a stack trace), the truncated text doesn't parse, so the code falls through to normalizeErrorDetail(text). The message then gets the raw JSON prefix ({"error_code":...,"message":...) instead of the message field. The PR description says the detail is the "JSON message, else plain text", and that doesn't hold for these bodies. Running JSON.parse on the full buffered body and applying the 4096 cap only in the plain-text fallback would keep that contract, at no extra cost since the body is already in memory.

[vuanhphung]

@bot
The cause's message becomes the full envelope JSON, including diagnosticInfo and the unbounded errorDetailsJson, and util.inspect prints a non-enumerable cause, so this now shows up for every kernel error. Could we wrap instead of mutating, e.g. new Error(${code}: ${msg}) with a bounded length?

[peco-engineer-bot]

Pushed cc1e213 to fix/issue-467-connection-failure-detail addressing your request.

Started from d9c01e4. decodeNapiKernelError no longer changes the binding error. cause is now a new non-enumerable Error(${code}: ${msg}), cut to 500 characters plus …. That keeps diagnosticInfo, errorDetailsJson and the sentinel out of util.inspect output. Updated the cause tests in error-mapping and auth-edge-cases, added a test for the length limit and the left-out diagnostics, and updated the CHANGELOG. build, the affected tests, npm test (1582 passing), type-check and prettier pass. npm run lint didn't run: ESLint couldn't find the airbnb-base config referenced from the parent-directory .eslintrc. The kernel e2e test was not run (no credentials).

[github-actions]

Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase (git rebase -i main).

[peco-review-bot]

✅ No issues identified by the review bot.