ClickHouse / ClickHouse/clickhouse-js

Mid-stream exception detection false-positives on any `\r\n` in the body (breaks streaming Parquet + CRLF CSV/TSV) and can hang the event loop

Open
#974 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
331
Forks
74
PR merge metrics
No merged PRs in 30d

Description

## Summary

The in-band mid-stream exception detector (for ClickHouse 25.11+, which appends a `__exception__` trailer after a 200 response when an error occurs mid-stream) is armed on **every** streaming query against a modern server: the `x-clickhouse-exception-tag` header is a *leading* header sent on successful responses too. The detector then treats **any** `\r\n` in the body as an exception trailer, without ever validating that the `__exception__` marker bytes are actually present.

Verified against a real ClickHouse **26.5.1.882** server.

## Defects

1. **False positive on any `\r\n` in the body.** The call sites (`packages/client-node/src/result_set.ts`, `packages/client-web/src/result_set.ts`) trigger `extractErrorAtTheEndOfChunk` whenever a `\n` is preceded by `\r`, but `extractErrorAtTheEndOfChunk` (`packages/client-common/src/utils/stream.ts`) never compares the `__exception__` marker — it only uses `EXCEPTION_MARKER.length`. Any legitimate `0d 0a` pair aborts a successful query with a bogus error.
- **Streaming Parquet** (`Parquet` is streamable and binary): a successful 289 KB response contained 1195 `0x0a` bytes, 5 preceded by `0x0d` → the detector fires (~1 false trigger per 64 KiB of binary data). Streaming Parquet is effectively always broken against 25.11+.
- **CRLF CSV/TSV** (`output_format_csv_crlf_end_of_line` / `output_format_tsv_crlf_end_of_line`): the server emits `0d 0a` per row, so row 1 trips it and the user gets a nonsense error instead of their data.
2. **Unbounded backward scan → event-loop hang.** In `stream.ts`, `do { --errMsgLenStartIdx; } while (chunk[errMsgLenStartIdx] !== NEWLINE);` has no floor. If no `\n` exists below the start index (a single long CRLF-terminated row whose only `\n` is the final byte, or a proxy-truncated trailer), the index runs negative forever; `chunk[-1]` is `undefined` (never a newline) and nothing throws, so the surrounding `try/catch` cannot rescue it — blocking the whole Node.js event loop.
3. **Cross-chunk trailer split** (secondary): the `idx >= 1` guard and end-of-chunk length math assume the trailer is fully contained in one chunk; a trailer split across chunks yields "failed to parse the message length" instead of the real server error. (Larger change; can be tracked separately.)

Affects both `@clickhouse/client` (Node.js) and `@clickhouse/client-web`.

## Fix direction

Make detection sound: require the exact `\r\n__exception__\r\n` bytes at the end of the chunk before treating it as an exception trailer (the tag is a random per-response token, so an exact match is a reliable discriminator), and floor the backward scan so a malformed trailer returns an error instead of hanging.

Contributor guide

Open the contributing guide

Research direction

Start with packages/client-common/src/utils/stream.ts, then inspect the call sites in packages/client-node/src/result_set.ts and packages/client-web/src/result_set.ts. Reproduce the CRLF and malformed-trailer cases, and consider the existing streaming test entry points. Done means only an exact __exception__ trailer is detected, backward scanning is bounded, and normal Parquet or CRLF-delimited responses no longer fail or hang.

Written by the indexing model from the issue text.

Assessment

Tech stack
nodejs, typescript
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.