Web UI prototype for framing formats - #110210
Draft
alexey-milovidov wants to merge 211 commits into
Draft
Conversation
…, logs, and profile events in the HTTP response stream A framing format multiplexes different response parts of the query in a single stream - chunks of data, totals and extremes, progress packets, profile events (metrics), and server logs - everything that the native protocol supports. Framing formats are independent of output formats: they encapsulate bytes produced by any output format. The framing format is selected by the new query setting `framing_output_format` and currently applies to the HTTP protocol. Implemented framing formats: - `None` - transparently routes everything applicable to the output format, so everything works as it is by default. - `EventStream` - frames packets as HTTP server-sent events (`text/event-stream`). - `JSONEachPacketBase64` - every packet is a JSON object; the formatted data is base64-encoded. - `JSONEachPacketString` - every packet is a JSON object; the formatted data is put into a string. The framing format works as a multiplexor: the output format writes into the framing format's payload buffer, and `IOutputFormat` notifies it on packet boundaries, which wraps everything accumulated since the previous boundary into a packet. The concatenation of the payloads of all `data`, `totals`, and `extremes` packets is exactly what the output format would have produced without framing. Auxiliary packets (progress, logs, profile events, exceptions) are represented as JSON. Server logs and profile events reuse `InternalTextLogsQueue` and `ProfileEvents::getProfileEvents` - the same mechanisms as the native protocol - newly attached for HTTP queries. Exceptions are always written as the last packet of the stream, so the client can parse the response as a stream of packets. Processing of multiple queries at once is out of scope of the first implementation, but the design allows extending every packet with the information about the query index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
When the last payload line has no trailing `\n` (for example `FORMAT JSON`), `find_first_symbols<'\n'>` returns `end`, and `pos = line_end + 1` formed a pointer past the one-past-the-end position - undefined behavior, even though the loop exited immediately afterwards. Stop when `line_end == end` instead. Addresses a review comment on #110127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…treaming `IFramingFormat` wrote each packet to `out` and called `out.next()`, which only flushes the outermost buffer. When the HTTP output is wrapped in compression (`wrapWriteBufferWithCompressionMethod` / a `WriteBufferWithOwnMemoryDecorator`), `next` leaves the compressed bytes in the nested buffer, so `data`, `progress`, and `log` packets could sit there until the end of the query instead of being delivered interactively. Add a `flushOut` helper that also flushes the nested buffer - mirroring `IOutputFormat::flushImpl` - and use it at every packet boundary and on finalize. Addresses a review comment on #110127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`IOutputFormat` notifies the framing format of a `Totals` / `Extremes` packet right after `consumeTotals` / `consumeExtremes`. That assumes the bytes are already in the payload buffer, which holds for row formats but not for `Template` (`areTotalsAndExtremesUsedInFinalize` is true): it stores totals and extremes and emits them later from `finalizeImpl`, where the framing format can no longer tell them apart from the main data and would mislabel them as `data` packets. Reject such formats in `setFraming` with a clear exception instead of producing wrong output. Addresses a review comment on #110127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… rejection The `GROUP BY ... WITH TOTALS` queries in `04512_framing_formats` emitted a nondeterministic number of `data` packets: a `data` packet follows every output block boundary, and the number of blocks depends on the number of threads, two-level aggregation, the output block size, and external sorting - all of which the CI settings randomizer varies. This made the test fail in the `arm_asan_ubsan` targeted run and in the `amd_debug` / `amd_asan_ubsan` / `amd_tsan` / `amd_msan` flaky checks. Pin the settings that determine the block boundaries for those queries so the result is a single block. Also add a case asserting that a framing format is rejected for `Template`, which defers totals and extremes to finalize. CI report: #110127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… and HTTP compression Cover the two remaining test suggestions from the review: - `EventStream` with a payload whose last line has no trailing newline (`FORMAT Values` produces `(0),(1),(2)`), exercising the end-of-buffer fix. - Framing over an `enable_http_compression` response, so a regression in the nested-buffer flush that leaves packets stuck in the compression buffer is caught. Related to #110127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `EventStream` and `JSONEachPacketString` framing formats embed the raw output bytes into the response as UTF-8 text: `EventStream` writes them into `data:` fields of a `text/event-stream` response, and `JSONEachPacketString` passes them through `writeJSONString`, which does not sanitize invalid UTF-8. A binary output format such as `Native` or `RowBinary` would therefore produce invalid SSE / JSON instead of a byte-preserving stream. Reject such combinations at framing selection time (mirroring the existing rejection of formats that defer totals and extremes) and point the user to `JSONEachPacketBase64`, which encodes arbitrary bytes safely. The output format is classified as text via its content type: text formats declare a charset (e.g. `text/tab-separated-values; charset=UTF-8`, `application/json; charset=UTF-8`), while binary formats use types such as `application/octet-stream` without a charset. Addresses the AI review blocker on #110127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The text framing formats (`EventStream`, `JSONEachPacketString`) embed the output payload as UTF-8 text, so an output format that can emit arbitrary bytes corrupts the stream. Binary formats (`Native`, `RowBinary`) were already rejected via their `application/octet-stream` content type, but raw passthrough formats slipped through: `RawBLOB` writes the column bytes verbatim while inheriting the default `text/plain; charset=UTF-8` content type, and the `TSVRaw` / `TabSeparatedRaw` / `LineAsString` / `Raw` family advertise a textual content type while calling `serializeTextRaw` (no escaping). Both can produce non-UTF-8 output, reproducing the invalid SSE / JSON stream the earlier review flagged. Content type alone is not a reliable signal for text-safety, so make it an explicit output-format capability: add `may_produce_raw_bytes` to the format registration (`markOutputFormatMayProduceRawBytes`) and mark the raw formats with it. `outputFormatProducesText` now rejects any format marked this way in addition to the content-type check. `JSONEachPacketBase64` is unaffected, since it encodes arbitrary bytes safely. Extended `04512_framing_formats` to cover rejecting `RawBLOB` (always raw), `TSVRaw` and `LineAsString` (text-labeled raw) for text framings, and to confirm `JSONEachPacketBase64` still carries `RawBLOB` output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`FramingFormatEventStream::writeDataFields` emitted one SSE `data:` field per `\n`-terminated line of the payload. The client reconstructs `event.data` by joining the values of consecutive `data:` fields with `\n` and then stripping a single trailing `\n` (per the server-sent events specification). For a payload that ends with `\n` - the common case for line-based formats such as `JSONEachRow`, `TSV`, or `CSV` - the stripped trailing `\n` was lost, so a payload like `"row\n"` was reconstructed as `"row"`. That broke the central framing contract that concatenating the payloads of the `data`, `totals`, and `extremes` packets reproduces the output of the format without framing. Emit an extra empty `data:` field when the payload ends with `\n`, so the trailing newline survives the reconstruction. Payloads without a trailing newline (for example `FORMAT JSON`) are unchanged. Extended `04512_framing_formats` with a regression that rebuilds `event.data` from a newline-terminated `EventStream` payload and compares it byte-for-byte with the unframed output, and updated the existing `EventStream` reference and the documentation example to reflect the trailing empty `data:` field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pulls in the merged fix #110162 (revert of EXPLAIN ANALYZE pipeline timing, #110162), which fixes the 'clock' logical error that failed the Stress test (amd_msan) on this PR: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110127&sha=e774374cff8ac2e6fb0e606fe60ed73930998360&name_0=PR&name_1=Stress%20test%20%28amd_msan%29
The `EventStream` (and other) framing formats sent fewer server log packets than the native protocol: `clickhouse-client` receives log entries from the start (parsing, planning, analysis - `executeQuery`, `Planner`, `SelectExecutor`, "Reading approx N rows") and the end (`executeQuery: Read N rows`, `MemoryTracker: Query peak memory usage`) of the query, while the framing format captured only the logs emitted during pipeline execution. Two causes: - The logs and profile-events queues were attached to the thread only after the query had been interpreted, so the interpretation-phase logs were lost. Attach them before interpretation (in `attachQueuesForFramingIfApplicable`), as the native protocol does, and wire them into the framing format once it is created. - The framing format was finalized during pipeline execution, before the query-finish logging ran, so the trailing logs were lost. Defer the framing finalization (`IOutputFormat::deferFramingFinalize`) until after `onFinish`, while the HTTP response stream is still open, and log the peak memory usage explicitly before the drain (mirroring `TCPHandler`). The exception path in `HTTPHandler` finalizes the deferred framing format explicitly. With this change the framing format streams the same set of server logs as the native protocol. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `EventStream` framing format previously rejected output formats that are not guaranteed to produce valid UTF-8 text (binary formats such as `Native` or `RowBinary`, and raw passthrough formats such as `RawBLOB` or `TSVRaw`), because server-sent events is a text protocol. Instead of rejecting them, `EventStream` now base64-encodes the `data`, `totals`, and `extremes` payloads (each into a single `data:` field) so that arbitrary bytes survive the text transport, and signals this by adding a `payload=base64` parameter to the `Content-Type` (`text/event-stream; charset=UTF-8; payload=base64`). The client base64-decodes those payloads; their concatenation is byte-for-byte what the output format would have produced without framing. Text output formats are still embedded as plain text, and the auxiliary JSON packets (progress, logs, profile events, exception) are never encoded. This lets progress, server logs, and profile events be streamed for any output format over `EventStream`. `JSONEachPacketString` still requires a text output format (it puts the bytes into a JSON string and cannot encode arbitrary bytes), and `Template` is still rejected for all framings because it defers totals and extremes to finalize. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rework the query progress display in the Web UI to consume the `EventStream` framing format over HTTP, so it works regardless of the output format (including `FORMAT Pretty`, binary formats via base64, and images). - Stream every query with `framing_output_format=EventStream` and `send_logs_level=trace`, decoding data/progress/log/profile_events/ exception packets as they arrive. - Show realtime CPU, memory, and disk usage like `clickhouse-client`, aggregated per host (total and max/host), switching to peak RAM when the query finishes. - Merge elapsed time and realtime metrics into a single element rendered on top of the progress bar; keep the text readable with a background-clip gradient that darkens the glyphs only over the colored fill (per-theme peak color and fraction). - Display server logs in real time, colored like the client, with a Logs button available even on exception; keep the view fast for 100k+ lines via batched appends and content-visibility while preserving native browser search. - Render inline images for image output formats, and base64-decode binary payloads signalled by the `payload=base64` content type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Iterate on the query progress display: - Merge the elapsed time, realtime resource metrics, and rows/bytes read stats into a single progress area so the progress-bar gradient renders behind all of them; the read stats stay right-aligned and wrap when long, the elapsed/metrics text keeps its natural width. - Draw the colored bar only one text line tall, so when the read stats wrap and grow the area the fill stays a single-line strip. - Tint every glyph (left metrics and right stats alike) with a text mask clipped from the same full-width `--progress` gradient as the bar, so the two correspond exactly and the text stays readable over the fill. Light theme uses a plain gray to black; dark theme uses a steep gray to white (first quarter) then a jump to black. - Use a 10pt font for the progress area. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
Contributor
|
Workflow [PR], commit [8bd6303] Summary: ⏳
|
clickhouse-gh
Bot
added
the
pr-not-for-changelog
…and framed exceptions Address the review findings on the framing formats (#110127): - `EventStream` now serializes a multi-row `profile_events` batch as a single JSON array in one `data:` field. An SSE client reconstructs `event.data` by joining consecutive `data:` fields with a newline, so emitting one field per row produced `{...}\n{...}`, which is not valid JSON and did not match the documented `profile_events` contract (an array of profile events as JSON). This mirrors the `profile_events` array of the `JSONEachPacket` framings. - Raw-byte detection for text framings is now settings-aware. `CustomSeparated` with `format_custom_escaping_rule = 'Raw'` writes the column bytes verbatim (like `TSVRaw`), so it may produce non-UTF-8 output even though it advertises a textual content type. Since this depends on a setting rather than the format name, it cannot be marked statically with `markOutputFormatMayProduceRawBytes`; a settings-aware checker (`registerOutputFormatMayProduceRawBytesChecker`) is added alongside the static flag. `EventStream` now base64-encodes such output, and `JSONEachPacketString` rejects it, pointing to `JSONEachPacketBase64`. - When the failure is the framing/output-format compatibility check itself (for example `JSONEachPacketString` with `FORMAT RowBinary`, or a framing with a format that defers totals/extremes such as `Template`), the error is now delivered as a framed `exception` packet instead of a plain HTTP error body. The exception packet is always JSON regardless of the output format, so the exception-recovery path creates the framing with a `for_exception` flag that skips the data-payload compatibility check and the deferred-totals check. - The exception-recovery formatter now wires in the logs and profile-events queues attached before the query is interpreted, so `log` / `profile_events` packets accumulated during parsing and planning are drained on `finalize` instead of being dropped when the query fails before producing output. - Update the `framing_output_format` setting description: `EventStream` no longer requires a text output format, it base64-encodes non-UTF-8 output. Extend `04512_framing_formats` and `04513_framing_formats_logs_profile_events` with regressions for each of the above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Replace the Result/Logs button pair with a single "Logs" toggle at the front of the progress line (just before the elapsed/metrics text). It switches the result element(s) between the query result and the streamed server logs, shows a dotted underline, uses the monospace font, and gets a yellow background while the logs view is active. - Show the toggle only when the query produced at least one log line, so it stays hidden when a query has not started or produced no logs. - Drop the now-meaningless leading `NN.N%, ` from the read stats once the query finishes (the percentage only matters while progressing to 100%). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
…xception path, document JSONEachPacketString UTF-8 contract Addresses the AI review of `3a52d17a`. `HTTPHandler` buffered exception path (`http_wait_end_of_query=1`): on an exception the buffered output is discarded and the framing format is recreated from `framing_name` alone, so the `log` and `profile_events` queues that `executeQuery` attaches before parsing (and wires into the framing) were dropped - `http_wait_end_of_query=1&framing_output_format=JSONEachPacketString&send_logs_level=trace` lost the parsing/planning `log` packets that the streaming path and the docs promise. The queues are now carried into the buffered exception writer: they are captured by value (the original framing object goes out of scope before the writer runs) and re-applied to the recreated framing via new `IFramingFormat` accessors. Added a `wait_end_of_query` regression to `04513_framing_formats_logs_profile_events`. `JSONEachPacketString` UTF-8 contract: documented that it puts the payload bytes into a JSON string without validating or re-encoding them, so text output formats such as `JSONEachRow`, `TSV` or `CSV` may emit invalid UTF-8 for `String` / `FixedString` values holding arbitrary bytes (just as ClickHouse's own `JSONEachRow` does with the default `output_format_json_validate_utf8 = 0`), in which case the NDJSON stream is not guaranteed to be valid UTF-8; pointed to `JSONEachPacketBase64` for byte-exact transport. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…m snapshot Address review (#110210): the history/tab replay path only distinguished the default (table) output from `rawText`; it never recorded whether the saved `EventStream` response used `payload=base64` (a binary or raw output format such as `Native` or `RawBLOB`). A restored snapshot of such a result was therefore replayed as literal base64 text, whereas the live `readEventStream` path decodes the payload to bytes and renders it as an image or raw text. Persist an `is_base64` bit with the result snapshot (single-query, multi-query, and the `saveHistory` / flat-entry serialization) and have `renderEventStreamText` decode the base64 `data`/`totals`/`extremes` payloads and hand them to `renderBinaryPayload`, exactly like the live path. A base64 snapshot is rendered as an image or raw text and is never finished as a table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keep the Web UI prototype current with master and clear the reported conflict with the base branch. The only conflict was a single hunk in `programs/server/play.html`, in the non-OK response branch of `postImpl`: master added query-error highlighting (`highlightQueryError`, guarded by `editorInteractionGen === runEditorInteraction`) while this branch added the `is_base64` field to the returned result and reworked the image `else if` condition to exclude framed (`text/event-stream`) responses. Resolved by keeping both: master's error-highlighting block, followed by this branch's `is_base64` return and framing-aware image branch. The `queryStart` argument that master threads into `postImpl` is already present (auto-merged), so the highlight call is in scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fective query settings The `log` / `profile_events` queues for a framing format were attached once, before the query was interpreted, from the pre-parse context. The framing decision (`createFramingFormatIfApplicable`), however, uses the effective settings after the query's own `SETTINGS` clause has been applied inside `executeQueryImpl`. The two could disagree: - A query that enables framing only in its own `SETTINGS` clause (`SELECT ... SETTINGS framing_output_format='JSONEachPacketString'`) still built a framed response, but the queues stayed empty, so no `log` / `profile_events` packets ever appeared. - The inverse override (framing or `send_logs_level` / `send_profile_events` enabled in the session / URL but turned off by the query's `SETTINGS` clause) still allocated the unbounded queues and captured packets that nobody drains. Replace `attachQueuesForFramingIfApplicable` with an idempotent `syncFramingQueuesWithSettings` that brings the attached queues into agreement with the current effective settings (attaching what is wanted, dropping what is not; the thread group keeps only a weak reference, so resetting the owning `shared_ptr` detaches the queue). It is called before the query is interpreted (so parse / plan-phase logs are still captured for framing requested from the session or URL) and again after `executeQueryImpl` has applied the query's `SETTINGS` clause, on both the success and the exception paths. A framing format enabled only by a query-level `SETTINGS` clause is not known before parsing, so its queues capture from query execution onwards - the parse / plan phase logs are captured only when framing is requested from the session or URL. Add regressions to `04513_framing_formats_logs_profile_events` covering query-level `SETTINGS` for `framing_output_format`, `send_logs_level`, and `send_profile_events`, including the inverse override. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o read-only queries Two fixes to the framing `EventStream` consumer in `play.html`, addressing the AI review of the PR: - `profile_events`: `EventStream` serializes each `profile_events` packet as a single JSON array of event objects in one `data:` field, but the client pushed every parsed line as one element, producing `[[...]]`. `feedProfileEvents` then saw the whole batch as a single element with no `thread_id` / `name`, so the realtime CPU / RAM / disk meters stopped updating for framed queries. Flatten the parsed batch into individual events (a bare object is tolerated too, for older one-per-line servers). - No-framing retry: the Web UI enables framing for every query, and a non-pulling pipeline (`INSERT`, DDL) never creates a framing stream, so such a statement can return a plain-HTTP error after it has already run. The previous code retried the whole request on any non-event-stream error, which could duplicate the side effect. Gate the retry to read-only queries (`queryIsReadOnly`, the same read-only kinds treated as parallelizable), so a side-effecting statement is never blindly resubmitted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ss` packet precedes the injected fault Framed `progress` packets are throttled by `interactive_delay` (100 ms by default). On a slow run (debug/sanitizer flaky checks), the failing query of the first case took longer than that, so a `progress` packet was emitted and flushed straight to the client (the response buffer is pinned to zero) before `framing_exception_packet_throw` fired, and the aborted response was not empty: 04817 failed with 'MISMATCH: something was delivered' in all four flaky checks. Pin `interactive_delay` to one hour on the streaming request, as 04628_framing_progress_throttle already does. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110127&sha=689e1df9b32e6c8b1bca628856d966a09740aefe&name_0=PR #110127
…format` setting description The description said every failed framed stream ends with an `exception` packet, while the implementation intentionally fails closed - terminating the stream without a terminal packet - once part of the packet stream has been produced and can no longer be discarded (a partial packet write, a failure of the exception delivery itself, or a failure while flushing or closing the response stream). `Settings.cpp` is the source for `system.settings` and the generated settings docs, so it over-promised the protocol contract. Carry the caveat from `docs/en/interfaces/framing-formats.md` into the description.
…k` and its query-log metrics semantics The comment in `executeQuery.h` still described `query_finish_callback` as a hook that runs before the `QueryFinish` entry is dumped to `query_log`, and the comment on the HTTP callback claimed the flush exists to attribute `NetworkSendElapsedMicroseconds` / `NetworkSendBytes` to the query. For a framed response both are stale: the framed stream must include the trailing `log` / `profile_events` packets emitted by the query-finish logging inside `onFinish` and end with the final `progress` packet, and closing the response stream must come after that, so the callback necessarily runs after the `QueryFinish` snapshot. The send counters of the response tail (and, for a buffered response, of the delayed-results push) are therefore not part of the query's snapshot - the same semantics as the native protocol, whose trailing log / profile-events sends after `logQueryFinish` are equally unattributed. Transmitting the buffered response before `onFinish` would forfeit the late-error contract instead: a failure in `onFinish` (a query-log write, for example) could no longer be delivered as a fresh framed `exception` response with a proper HTTP status (see 04817), which is the purpose of `wait_end_of_query`. Document the actual contract in `executeQuery.h`, `HTTPHandler.cpp`, and `framing-formats.md` instead of restructuring the ordering.
Member
Author
|
🕵 CI triage for
|
The WASM `tokenize` placed its buffers (the lexer object, the query bytes and the token out-pointers) starting at memory offset 0, but the module's own shadow stack occupies the low memory up to `__heap_base` and its call frames grow down from there - so once the query pushed the buffers into the frame region (~64 KiB), the module's calls and the buffers overwrote each other, and the corrupted token stream (garbage types, never end-of-stream, never an error) made the loop spin forever: with `detectFramingSetting`, `detectExplicitFormatClause`, `queryIsReadOnly` and `splitAllQueries` on the mandatory request path, a 64 KiB-class query hung the tab before any request was sent. The old 64 KiB `max_query_size` additionally flagged every token crossing that boundary as an error, silently truncating the token stream of a big query. Now the buffers start at `__heap_base`, the memory grows to fit the text, the lexer is created without a size cap, and a token stream that stops advancing throws instead of looping. The request-path detectors go through the new `tokenizeOrNull`, which translates any tokenizer failure (WebAssembly unavailable, growth failure, the progress guard) into their best-effort text-match fallbacks, so such a query still runs. Validated by driving the real extracted page functions in Node: the previously hanging ~64 KiB comment queries now tokenize completely; a framing setting, a `FORMAT` clause, a read-only classification, a `Run all` split, a query parameter and the query under cursor are all resolved past the old window; multi-megabyte queries tokenize (twice - the grown memory is reused); and with an injected tokenizer failure all five detectors fall back to their text-match heuristics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ession also fails closed The fail-close guard in `trySendExceptionToClient` was reported to miss framed bytes that a transport compressor (e.g. `BrotliWriteBuffer`) has already consumed into its internal codec state. It does not: `count` is cumulative (`bytes + offset()`, with `bytes` bumped by `WriteBuffer::next` even when `nextImpl` throws), and `out_maybe_compressed` is the buffer the framing writes into - the topmost of the compression wrapper, the internal `compress=1` layer and the response buffer - so bytes can only reach the codec state through a buffer whose `count` is then permanently non-zero. Extend the comment to state this, and add a regression to `04817_framing_error_with_buffered_response` that runs the framed exception delivery failure under `Accept-Encoding: br` with `enable_http_compression=1` and asserts the client never observes a complete response with a plain error body appended. #110127
…ls on an absent file In the Fast test environment the aborted compressed response delivered no bytes at all, so `curl -o` never created the output file and the subsequent `grep` spilled "No such file or directory" to stderr, failing the test despite all assertions passing. Pre-create the file empty: an empty response trivially contains no plain error body. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110127&sha=066f27a9704c2627bdc8f0d4892aa142a4aeb381&name_0=PR&name_1=Fast%20test #110127 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
🕵 CI triage for Related: #113292 @groeneai, no fix PR exists yet for #113292 — please provide a fix in a separate PR (or link it here if already in progress). Meanwhile I merged the latest |
Contributor
|
A fix PR exists and has been open since 2026-08-05: #113515, which carries It is the same defect as the fuzzed What it needs is a reviewer. CI is finished on On the triage flow: this is the third time you have asked me for #113292, and I linked #113515 in reply both previous times (#42701 on 08-07, #112152 on 08-08). A check for an open PR whose body links the issue would drop the round trip. |
…explicitly
The string fields of the auxiliary framed packets (`log`, `profile_events`,
`exception`) are written through `WriteBufferValidUTF8`, whose tail stays
buffered until the buffer is finalized. `writeJSONStringValidUTF8` left that to
the destructor, which catches and suppresses any exception from `finalize`
(`WriteBufferValidUTF8::~WriteBufferValidUTF8`). Since the buffer writes
straight into the live response stream, a failure to write the last bytes of
such a string was swallowed: the packet was left truncated - hence invalid
JSON - on the wire, while `emitToOut` cleared `writing` and the stream happily
continued to its success terminator.
`finalize` is now called explicitly, so such a failure propagates into the
existing fail-close path: the stream ends with the half-written packet and
carries no terminator at all. `WriteBuffer::finalize` cancels the buffer when
`finalizeImpl` throws, so the destructor adds nothing after that.
Added the `write_buffer_valid_utf8_finalize_throw` fail point (the failure has
to come out of the flush itself, which is only injectable there) and the
regression test `04839_framing_string_flush_error`, covering both a
`profile_events` packet of a successful query and the terminal `exception`
packet. Verified that the test fails without the fix: the previous behavior
delivered `"host_name":,` followed by the final `progress` packet with
`result_rows`, and `{"packet":"exception","exception":}` as a complete
response.
…et_result_details` Two review findings on the framed error paths. 1. `HTTPHandler`'s fail-close latch for framed responses (`used_output.framed`) was only set from the `set_query_result` callback. On the exception path `executeQuery` deliberately swallows a failure of that callback (see the `execute_query_calling_empty_set_result_func_on_exception` failpoint), so the callback may never run even though the response is then written through a framing format. A second failure - while delivering the exception packet or while closing the response - would then make `trySendExceptionToClient` miss the framed fast path and append the generic `__exception__` block after a partial packet stream. `handle_exception_in_output_format` now latches the flag itself whenever the output format has a framing. 2. The `JSONObjectEachRow` raw-bytes checker only looked at the header names, but the values of the column selected by `format_json_object_each_row_column_for_object_name` become the outer JSON keys and are written verbatim - `JSONUtils::writeCompactObjectStart` emits the title with `writeCString`, without escaping or UTF-8 validation. A value containing a quote, a newline or non-UTF-8 bytes therefore makes the output non-textual, which is data-dependent and not knowable from the header. The checker now fails close whenever such a column is selected, so the text framings reject it or switch to a base64 payload. New test `04843_framed_fail_close_without_result_details` covers (1); `04512` covers (2) - its previous "object-name column is not checked" case is replaced by the rejection, a byte-exact base64 round trip of an output whose object name breaks JSON, and the acceptance of the synthesized `row_N` object names.
…xception The test now exercises the fail-close contract of a framed exception response whose `set_result_details` callback never ran for both injectable secondary failures: a throw while the terminal `exception` packet is delivered (`framing_exception_packet_throw`, the response is cut before any bytes reach the client) and a throw while the response is closed (`http_output_finalize_throw`, the complete packet stream is on the wire). In both cases nothing may follow the packet stream: no generic `__exception__` block, no second `exception` packet, and the client observes an aborted connection. The first section's earlier assertion that `log` packets are already on the wire was wrong: with a query that fails during analysis, the auxiliary packets of the recovery stream are still buffered when the injected failure hits, so the client sees an empty response.
…tion packet
Three findings on the Web UI request/response handling, plus the review's test
request for the SQL-lexer ports.
1. `detectFramingSetting` only opened a settings context when the list began with
a `name = value` entry. `ParserSetQuery` also accepts valueless shorthand
settings, so a valid query like `SELECT 1 SETTINGS optimize_move_to_prewhere,
framing_output_format = 'None'` was skipped entirely and the page added its own
`EventStream` framing instead of honoring - or refusing - the query's choice.
The walk now consumes shorthand entries. Because that token shape also matches
an ordinary column named `settings` (`SELECT settings x, framing_output_format =
'None' FROM t`), a list opened by a shorthand entry alone is trusted only when
it also ends where a settings list can end (end of query, `;`, or a clause that
may follow the list); otherwise the walk is rolled back.
2. The framing-compatibility retry matched `is not compatible with framing
formats` anywhere in the buffered error stream. A failed stream can already
carry `data` and `log` packets, so a row or log line containing that phrase
resubmitted the query and discarded the partial stream that had been rendered.
The decision now comes from the message of the terminal `exception` packet only
(`framedFailureMessage`).
3. The non-OK `application/x-ndjson` branch assumed that content type always means
a packet stream. Plain `JSONEachRow` / `JSONLines` / `NDJSON` use it too and,
with `http_write_exception_in_output_format`, return a non-OK `{"exception":...}`
body that the generic error branch renders properly - while this branch echoed
the raw line and skipped the error rendering. The branch is now gated on
`ndjson_exception_prefix !== null`, i.e. on the cases that really are
packetized.
The lexer ports in `src/Parsers/tests/gtest_play_*` created `DB::Lexer` with the
old 64 KiB `max_query_size` cap, so they analyzed only a truncated prefix of a
large query and could not cover the no-limit tokenizer the page now uses. They
create it without a limit and report an unexpected error token as a test failure
(except the `getQueryUnderCursor` port, where stopping at an error token IS the
behavior under test). Each of the three request-path ports gained a `>64 KiB`
regression, and `gtest_play_detect_framing_setting` covers the shorthand walk and
its rollback. `04548` pins the new page invariants and the two server contracts
behind them: a plain NDJSON format's non-OK in-band exception body, and a settings
list that begins with a shorthand entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prototype changes to the Web UI (
programs/server/play.html) to test and showcase the framing formats feature. This is a draft for experimentation and review of the client-side experience, not intended to merge as-is.It builds on the framing-formats server work in #110127 (this branch is based on that PR, so the diff includes those commits until it merges into
master).What the prototype does:
EventStreamframing format over HTTP (framing_output_format=EventStream,send_logs_level=trace), decoding data / progress / log / profile_events / exception packets as they arrive - so progress and logs work regardless of the output format (includingFORMAT Pretty, binary formats via base64, and images).clickhouse-client, aggregated per host (total and max/host), switching to peak RAM when the query finishes. The meter state is owned by each tab: the CPU counters inprofile_eventspackets are per-packet increments, so a query running in a background tab keeps accumulating them, and reopening the tab continues the meter from its live values (rather than restarting near zero).event: exceptionblock, the{"packet":"exception",...}line, or the plain{"exception":...}line, captured at stream-read time since the capped text stops growing before the terminal exception arrives), so reopening or reloading a failed tab still shows the error reason.data/totals/extremespayload (theEventStreamwire encodes each block as a single base64data:field and theContent-Typecarriespayload=base64), deciding the rendering by the output format: the default format is reassembled into the table, an image format (or bytes carrying an image signature) is rendered as an inline image once the stream completes, and any other format is decoded and shown incrementally as raw text.FORMATclause: the framed request asks forJSONCompactStringsEachRowWithNamesAndTypes(the framing rejects the in-band-progressJSONStringsEachRowWithProgress) and the client reassembles the compact rows into the table renderer's shapes; on the server side (Framing formats: multiplex data, totals, extremes, progress, logs, and profile events in the HTTP response stream #110127) the compact family emits totals and extremes under framing, soWITH TOTALSand extremes-based column coloring keep working on the default path. When such a format is shown as raw text instead (an explicitFORMAT JSONCompactEachRow), only thedatapackets are concatenated, so the rendered text is exactly the plain output of that format rather than one carrying the totals and extremes rows the format itself drops.exceptionpacket as a query failure even when the HTTP status was already 200 (soRun allstops at the failed statement) - and, when a query chooses its ownJSONEachPacket*framing that fails before the 200 OK header (coming back as a non-200application/x-ndjsonpacket stream ending with a{"packet":"exception",...}line), shows those packets verbatim and recordsframing_kind = 'ndjson_packets'for replay rather than rendering the whole stream as one opaque error string; retries framing-incompatible explicit formats (e.g.FORMAT JSONEachRowWithProgress,FORMAT Template) once without framing for read-only queries (read-only-ness is resolved with a CTE-aware lexer walk -WITH y AS (SELECT 1) INSERT INTO t SELECT * FROM yis a write - shared withRun all's grouping, so such a statement is also a barrier there and never runs in parallel with the reads that follow it; the port with regression coverage lives insrc/Parsers/tests/gtest_play_query_is_read_only.cpp); a retriedJSON*EachRowWithProgressformat that itself reports a failure in-band - a trailing{"exception":...}object while the HTTP status stays 200 (http_write_exception_in_output_format) - is detected as a failure too, keyed off the output format rather than only a user-chosenJSONEachPacket*framing, soRun allstops after such a retried query fails; and does not add its own framing to a query that sets its ownframing_output_formatto a real framing choice (the response is then dispatched by content type, so the requested packets are shown verbatim); a query that setsframing_output_format = 'None'is refused client-side instead, since this page's rendering depends on framing (values the page cannot know upfront -= DEFAULT, a reset to the session/server default, and query-parameter placeholders like= {fmt:String}- are classified conservatively the same way and refused, rather than sent with a request shape the response might not match); likewise a standaloneSET framing_output_format = ...is refused, because it would change the setting for the whole session (with asession_id) while the page keeps adding its own framing per request - a query-levelSETTINGS framing_output_format = ...clause is the supported way to choose framing for one query. A query that carries its ownframing_output_formatis also refused for download (the setting would override the download's chosendefault_format, so the file would be the framing packet stream), and if such a query fails, its history snapshot - the rawJSONEachPacket*packet stream - is replayed as raw text on tab-switch/reload rather than as one opaque error string.framing_output_format=Noneon every request that expects an unframed response (the plain/chart request, the compatibility retry, the download, and the panel/server-status/completion queries), rather than only omitting the setting - otherwise a framing carried by the connection URL or by the HTTP session behind it would frame those responses too. A query-levelSETTINGS framing_output_format = ...clause is applied after the URL parameters, so a query that intentionally chooses a framing still overrides the pin.event_stream/ndjson_packets/ none) with each result snapshot and keys the history/tab replay off it, instead of guessing from the payload's first bytes - so a raw result whose text happens to start withevent:(e.g.SELECT 'event: data' FORMAT RawBLOB) or{"packet":is not reparsed as a framing stream after a tab switch or reload. A snapshot recorded asndjson_packetsis replayed as raw text regardless of whether the run succeeded and of its underlying output format, matching the live path - so a successful user-framedJSONEachPacket*result whose format has its own restore path (a table or aJSONCompactColumnschart) is not reparsed as that format's JSON. The snapshot also records whether the framed stream was truncated, so replay keeps the live fail-closed behavior for images: a cut-off framedFORMAT PNGresponse that showed only its error live reopens from history / Back / Forward as that error too, never as a partially decoded picture (a failed but complete stream - a terminalexceptionpacket after the payload - still renders its collected image, as live).FORMATclause and itsframing_output_formatsetting with the WASM lexer rather than a raw text match, so a mention inside a string literal or a comment - e.g.SELECT 'FORMAT JSONCompactColumns'- does not make the page silently opt out of its own framing. The detection is positional, not keyword-adjacent: a settings context is recognized by itsname = valuelist grammar (a column merely namedsettingsdoes not open one), and aFORMATclause candidate must follow a token that ends an expression (so inWITH 1 AS format SELECT format JSONCompactColumns SETTINGS max_threads = 1bothformatwords are identifiers, not a clause). The download reuses the sameFORMAT-clause detection (now returning the clause span) to strip only a real trailingFORMATclause from the download query, leaving text or ordinary SQL likeSELECT 'FORMAT TSV' AS suntouched. Both detectors also accept a quoted spelling of the name - the server parses setting names andFORMATnames with identifier parsers, so a backquotedframing_output_formator format name is real - comparing by the unquoted name. Regression coverage for both lives insrc/Parsers/tests/gtest_play_detect_explicit_format.cppandgtest_play_detect_framing_setting.cpp(ports of the token walking onto the realDB::Lexer).FormatFactorylooks them up by their lowercased name), whileX-ClickHouse-Formatechoes the identifier exactly as theFORMATclause spelled it, soFORMAT jsoncompactcolumnsused to lose the chart renderer, a lowercased default format lost the table renderer, and the late in-band exception probes did not recognizeFORMAT xml/FORMAT json/FORMAT jsoneachrowwithprogress- a query failing after its200 OKheader was then reported as a success andRun allcontinued past it. Every dispatch now compares a lowercased copy of the format name.SELECTqueries now also end with the documented finalprogresspacket carryingresult_rows/result_bytes/memory_usage, matching the native protocol and the no-result path.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Prototype Web UI changes to test framing formats (draft).
Related: #110127