Fix streaming: make mid-stream exception detection sound (no false positives, no event-loop hang)

[polyglotAI-bot]

Description

Fixes #974.

ClickHouse 25.11+ appends an in-band __exception__ trailer to a 200 response when a query errors after streaming has begun, and echoes a random per-response token in the x-clickhouse-exception-tag header. That header is a leading header sent on successful responses too, so the detector in ResultSet.stream() is armed on every streaming query. The detector then treated any \r\n in the body as an exception trailer — extractErrorAtTheEndOfChunk only uses EXCEPTION_MARKER.length and never compares the marker bytes — so a stray 0d 0a aborted a successful query with a bogus error. Separately, its trailer-length backward scan (do { --i } while (chunk[i] !== NEWLINE)) had no floor, so a trailer with no interior newline ran the index negative forever, blocking the Node.js event loop (a busy loop, so the surrounding try/catch could not rescue it).

Verified against a real ClickHouse 26.5.1.882 server: the real trailer ends the body with the exact bytes <tag>\r\n__exception__\r\n.

Changes

• packages/client-common/src/utils/stream.ts
  • Add endsWithExceptionMarker(chunk, exceptionTag) — a sound discriminator that returns true only when the chunk ends with the exact <tag>\r\n__exception__\r\n bytes. Requiring both the fixed __exception__ marker and the random per-response tag makes a stray \r\n in a successful body (binary Parquet, CRLF-terminated CSV/TSV) no longer a false positive. Guards the empty-tag precondition and does a byte-exact, allocation-free comparison.
  • Floor the backward scan in extractErrorAtTheEndOfChunk so a malformed / proxy-truncated trailer returns an error instead of spinning forever.
• packages/client-node/src/result_set.ts and packages/client-web/src/result_set.ts — gate the exception path on endsWithExceptionMarker(...) (the existing \r-before-\n check is kept as a cheap pre-filter). The two ResultSet implementations are intentionally duplicated, so the change is applied identically to both.
• packages/client-common/src/index.ts — export the new helper.
• CHANGELOGs for @clickhouse/client and @clickhouse/client-web (shared common-module change affects both).

The cross-chunk trailer-split robustness (a trailer split across two chunks) is a larger change and is intentionally out of scope here; it is noted in the issue for separate tracking.

Test

• packages/client-common/__tests__/unit/stream_utils.test.ts — parametrized unit tests for endsWithExceptionMarker (real trailer → true; CRLF CSV/TSV, embedded-\r\n binary, too-short chunk, exact-length-minus-one boundary, wrong tag, empty tag → false), plus a regression that extractErrorAtTheEndOfChunk returns an error instead of hanging on a trailer with no length delimiter.
• packages/client-node/__tests__/unit/node_exception_tag.test.ts and packages/client-web/__tests__/unit/web_exception_tag.test.ts — live-path regressions through the real ResultSet.stream() transform: a successful CRLF CSV stream completes, a binary body containing \r\n completes, and a genuine mid-stream exception still surfaces the real server message (no-regression guard).

The Node and common suites are green (test:node:unit), plus typecheck and lint --max-warnings=0. The Node live-path tests fail on unpatched main for the right reason (the detector aborts the CRLF/binary stream); they pass with the fix. The Web live path was verified equivalently in Node against the built @clickhouse/client-web (the Web browser test runner needs a browser image not available in the dev sandbox).

Pre-PR validation gate

• Deterministic repro confirmed (real 26.5.1.882 server + unit repro)
• Root cause documented above
• Fix targets the root cause (sound marker match + bounded scan)
• Tests fail without fix, pass with fix
• No existing tests weakened or edited (new tests only)
• Convention compliance verified per packages/AGENTS.md (CHANGELOG in both affected packages, prettier, lint)
• Fix on the live runtime path (proven via ResultSet.stream() end-to-end tests)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR fixes ClickHouse 25.11+ mid-stream exception detection in ResultSet.stream() by making trailer detection byte-exact (tag + __exception__ marker) to avoid false positives on \r\n inside successful response bodies, and by bounding a backward scan that previously could busy-loop and hang the event loop on malformed trailers.

Changes:

• Add endsWithExceptionMarker(chunk, exceptionTag) and use it to confirm real exception trailers before aborting streaming.
• Bound the backward scan in extractErrorAtTheEndOfChunk to prevent infinite loops on malformed/proxy-truncated trailers.
• Add targeted unit and “live-path” regression tests; update both Node and Web package changelogs.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/client-common/src/utils/stream.ts	Floors the backward scan in trailer parsing; adds endsWithExceptionMarker discriminator.
packages/client-node/src/result_set.ts	Gates mid-stream exception handling on endsWithExceptionMarker.
packages/client-web/src/result_set.ts	Mirrors Node gating for Web ResultSet.stream() implementation.
packages/client-common/src/index.ts	Re-exports endsWithExceptionMarker from the common bundle.
packages/client-common/tests/unit/stream_utils.test.ts	Adds unit tests for endsWithExceptionMarker + regression for bounded scan.
packages/client-node/tests/unit/node_exception_tag.test.ts	Adds end-to-end-ish regression tests for Node streaming path.
packages/client-web/tests/unit/web_exception_tag.test.ts	Adds equivalent regression tests for Web streaming path.
packages/client-node/CHANGELOG.md	Adds a bug-fix entry documenting the streaming exception detector fix.
packages/client-web/CHANGELOG.md	Adds the matching bug-fix entry for the Web package.

[polyglotAI-bot]

Thanks — I checked this against the exact scenario, both on a live 26.5 server and in a unit test.

1. The surfaced error is the real server error, not a row-1 error. extractErrorAtTheEndOfChunk always parses the trailer at the end of the chunk, independent of which newline iteration triggers the check. So even when the \r-before-\n pre-filter matches at the first CRLF row, the Error handed to callback is the genuine server exception. I added a regression test (node_exception_tag.test.ts → "surfaces the real exception message when preceding rows are CRLF-terminated"): it streams 0\r\n1\r\n2\r\n followed by a real throwIf trailer and asserts the actual FUNCTION_THROW_IF_VALUE_IS_NON_ZERO message surfaces — which it does, with no hang.

2. Dropping the rows accumulated in the terminal chunk is pre-existing behavior, unchanged by this PR. The only change this PR makes to result_set.ts is adding the && endsWithExceptionMarker(chunk, exceptionTag) conjunct to the existing condition; the return callback(err) (which returns before pushing rows) is exactly as it was before. When a mid-stream exception occurs the query has failed, so — like rows from earlier chunks that were already delivered — the client surfaces the error and discards the incomplete tail rather than handing back a partial result.

Detecting at the trailer position rather than the first qualifying CRLF, and flushing partial rows, is the same larger robustness area as the deferred cross-chunk-split handling, and is intentionally out of scope here — this PR's goal is narrowly to make detection sound (kill the false-positive aborts and the event-loop hang). Happy to open a separate issue if you'd like the failed-query partial-row semantics reconsidered. Leaving this thread for your call.

[polyglotAI-bot]

Thanks — I checked this against the exact scenario. The Web path uses the same shared endsWithExceptionMarker / extractErrorAtTheEndOfChunk, so the same reasoning applies:

1. The surfaced error is the real server error, not a row-1 error. extractErrorAtTheEndOfChunk always parses the trailer at the end of the chunk, independent of which newline iteration triggers the check. So even when the \r-before-\n pre-filter matches at the first CRLF row, the Error passed to controller.error(err) is the genuine server exception. I added a regression test (web_exception_tag.test.ts → "surfaces the real exception message when preceding rows are CRLF-terminated") and verified the built web ResultSet end-to-end: streaming 0\r\n1\r\n2\r\n + a real throwIf trailer surfaces the actual FUNCTION_THROW_IF_VALUE_IS_NON_ZERO message (and a successful 0\r\n1\r\n2\r\n body still completes with 3 rows), with no hang.

2. Dropping the rows accumulated in the terminal chunk is pre-existing behavior, unchanged by this PR. The only change this PR makes to result_set.ts is adding the && endsWithExceptionMarker(chunk, exceptionTag) conjunct to the existing condition; the controller.error(err) (which returns before enqueuing accumulated rows) is exactly as it was before. When a mid-stream exception occurs the query has failed, so — like rows from earlier chunks that were already enqueued — the client surfaces the error and discards the incomplete tail rather than handing back a partial result.

Detecting at the trailer position rather than the first qualifying CRLF, and flushing partial rows, is the same larger robustness area as the deferred cross-chunk-split handling, and is intentionally out of scope here — this PR's goal is narrowly to make detection sound (kill the false-positive aborts and the event-loop hang). Happy to open a separate issue if you'd like the failed-query partial-row semantics reconsidered. Leaving this thread for your call.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

packages/client-node/src/result_set.ts:260

• endsWithExceptionMarker(chunk, exceptionTag) is called inside the per-newline scan. For CRLF row formats this condition is evaluated for every row terminator, redoing the same O(tag+marker) suffix comparison repeatedly for the same chunk. Since the trailer (when present) is only ever at the end of the chunk, you can avoid the repeated work by only running the check when the current idx is the last byte in the chunk.

            if (
              exceptionTag !== undefined &&
              idx >= 1 &&
              chunk[idx - 1] === CARET_RETURN &&
              endsWithExceptionMarker(chunk, exceptionTag)

packages/client-web/src/result_set.ts:208

• endsWithExceptionMarker(chunk, exceptionTag) is called inside the per-newline scan. For CRLF row formats this condition is evaluated for every row terminator, redoing the same O(tag+marker) suffix comparison repeatedly for the same chunk. Since the trailer (when present) is only ever at the end of the chunk, you can avoid the repeated work by only running the check when the current idx is the last byte in the chunk.

            if (
              exceptionTag !== undefined &&
              idx >= 1 &&
              chunk[idx - 1] === CARET_RETURN &&
              endsWithExceptionMarker(chunk, exceptionTag)