fix(promql): malformed JSON from /api/v1/query_range on empty aggregations#103286
Draft
JTCunning wants to merge 7 commits intoClickHouse:masterfrom
Draft
Conversation
Summary
-------
The Prometheus query API endpoints (/api/v1/query, /api/v1/query_range)
returned an unbalanced JSON body when an aggregation over a non-existent
metric reached the timeSeriesFromGrid step function. Reproducer:
END=$(date -u +%s); START=$((END-900)); STEP=15
curl -sS "http://127.0.0.1:9093/api/v1/query_range" \
--data-urlencode "query=count(nonexistent_metric_name)" \
--data-urlencode "start=$START" --data-urlencode "end=$END" \
--data-urlencode "step=$STEP"
returned HTTP 400 with body:
{"status":"success","data":{"resultType":"matrix","result":[
{"status":"error","errorType":"bad_data",
"error":"Number of values (0) doesn't match number of steps (61):
while executing 'FUNCTION timeSeriesFromGrid(...)'"}
The outer '{', the data '{', and the result '[' are all left unclosed.
The Prometheus Go client (v1.apiResponse.Data) errors with
'ReadArrayCB: expect ] in the end, but found <EOF>'. All five trivial
aggregations (count/sum/avg/min/max) over a non-existent metric hit
this, as does any case where the inner aggregation produces zero rows
but timeSeriesFromGrid is still asked to emit N steps.
Two cooperating defects, both fixed here:
1. Function-level. timeSeriesFromGrid threw
"Number of values (0) doesn't match number of steps (N)" for any row
whose values array was empty. The Prometheus contract for
query_range over an empty aggregation is an empty matrix series, not
a hard error. The function now treats num_values == 0 as "no samples
for this row" and emits an empty per-row time series. The truly-
mismatched case (0 < num_values != num_steps) still throws
BAD_ARGUMENTS, preserving the existing strict contract.
2. Handler-level. The PromQL HTTP handler streamed the success envelope
('{"status":"success","data":{"resultType":"matrix","result":[')
directly into the response output stream, then on exception appended
a second '{"status":"error",...}' object onto the still-open buffer,
which is what produced the unbalanced body. The handler now buffers
the body in a WriteBufferFromString and only writes it to the HTTP
output stream on success; on exception the partial body is discarded
and a single, well-formed '{"status":"error",...}' document is
emitted with HTTP 400. As a defense-in-depth correctness fix the
error message is now JSON-escaped (it embeds SQL fragments that can
contain quotes/backslashes which would otherwise produce another
class of invalid JSON).
Either fix on its own defends against this class of bug; both are
landed because the function-level fix restores the regressed PASS for
the specific aggregation queries, while the handler-level fix is a
defense-in-depth so any future PromQL function failure mid-stream
cannot produce malformed JSON.
Tests
-----
- tests/queries/0_stateless/03254_timeseries_range.sql gains three
cases right after the existing wrong-count BAD_ARGUMENTS block:
empty Array(Nullable(Float64)), empty Array(Float64), and a
multi-row case mixing empty and populated arrays via arrayJoin to
prove an empty row does not poison sibling rows in the same block.
The existing BAD_ARGUMENTS cases are unchanged.
- tests/integration/test_prometheus_protocols/test_evaluation.py
gains test_query_range_empty_aggregation_returns_valid_json which
issues count/sum/avg/min/max(nonexistent_metric_name) over a
15-minute / 15-second-step range against the local ClickHouse
Prometheus endpoint and asserts the body parses as JSON in either
the success-with-empty-matrix or structured-error shape.
Verified end-to-end against a live ClickHouse instance: all five
reproducer queries now return HTTP 200 with
{"status":"success","data":{"resultType":"matrix","result":[]}};
parse-time and other mid-stream error paths now return clean
{"status":"error","errorType":"bad_data","error":"..."} envelopes
with HTTP 400.
Made-with: Cursor
Contributor
|
Workflow [PR], commit [782e489] Summary: ❌
AI ReviewSummaryThis PR fixes malformed ClickHouse Rules
Final Verdict
|
JTCunning
added
comp-promql
…y-api-malformed-json-on-empty-aggregation
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
Address clickhouse-gh AI Review on PR ClickHouse#103286: the previous fix wrapped the entire /api/v1/query and /api/v1/query_range response body in a String + WriteBufferFromString before writing it to the socket, which turned a network-streamed payload into in-memory accumulation and a peak-memory regression for large query_range responses (many series x many steps), with avoidable OOM risk under concurrency. Restore streaming, but keep the malformed-JSON guarantee that motivated the original fix: - PrometheusHTTPProtocolAPI::writeQueryResponse now performs the first PullingPipelineExecutor::pull(...) BEFORE emitting any byte of the '{"status":"success","data":{...,"result":[' envelope. The dominant PromQL evaluation failure mode (e.g. timeSeriesFromGrid over an aggregation that produced zero samples) raises on this first pull because the underlying pipeline is lazy, so it propagates with the response stream still untouched. - QueryAPIImpl::handlingRequestWithContext writes directly into the HTTP output buffer again. On a startup-side throw the headers have not been committed yet, so the catch block can still set HTTP 4xx and emit a single, well-formed JSON error document (with the error text properly JSON-escaped via writeJSONString). Net effect: bulk row data streams to the network as before (no full- response RAM accumulation), and the empty-aggregation regression covered by tests/integration/test_prometheus_protocols/test_evaluation still produces a valid JSON error body instead of a half-success / half-error concatenation. Made-with: Cursor
… test Address the remaining clickhouse-gh AI Review items on PR ClickHouse#103286: ❌ Blocker — src/Storages/TimeSeries/PrometheusHTTPProtocolAPI.cpp:114 The previous revision only deferred the success envelope past the *first* PullingPipelineExecutor::pull(). A throw from a *subsequent* pull (later block) would still land in QueryAPIImpl's catch block and append a second '{"status":"error",...}' object onto a response that already started with the success envelope, regressing to the exact concatenated-JSON shape this PR exists to prevent. QueryAPIImpl::handlingRequestWithContext now branches on `body_buf.count() == 0`: - Pre-output (count == 0): same as before — set HTTP 4xx and emit a single, well-formed structured-error JSON document. - Post-output (count > 0): re-throw. The outer PrometheusRequestHandler::handleRequest catch invokes WriteBufferFromHTTPServerResponse::cancelWithException, which appends a __exception__ marker block in a new chunk and intentionally skips the final empty chunk to break HTTP. The client sees a connection- level abort instead of concatenated JSON — wire format stays well-defined and no second JSON document is ever written.⚠️ Major — tests/integration/test_prometheus_protocols/test_evaluation.py:2763 The empty-aggregation regression guard accepted both `status == "success"` and `status == "error"`, so it would silently re-pass if the function-level fix regressed back to throwing. Split into two strict tests: - test_query_range_empty_aggregation_returns_empty_matrix Asserts HTTP 200 + `status == "success"` + empty matrix for count/sum/avg/min/max(nonexistent_metric_name). This is the strict regression guard for the timeSeriesFromGrid empty-array fix. - test_query_range_invalid_promql_returns_structured_error Asserts the structured-error envelope (`status == "error"` with `errorType` and `error`) for a deterministically broken PromQL expression. This covers the handler's pre-output error-path contract independently from the function-level guarantee. Made-with: Cursor
Address clickhouse-gh AI Review on PR ClickHouse#103286 (⚠️ Major): test_query_range_invalid_promql_returns_structured_error previously validated only the JSON envelope, so it would still pass if the handler regressed to returning HTTP 200 (or any non-4xx) with the same error shape — masking a real status-code regression for parse failures. Lock both layers at once: - assert response.status_code == 400 - assert body["errorType"] == "bad_data" - keep the existing status/error-field shape assertions Both invariants are guaranteed by QueryAPIImpl::handlingRequestWithContext in src/Server/PrometheusRequestHandler.cpp on the pre-output error path (setStatusAndReason(HTTP_BAD_REQUEST) + the literal "errorType":"bad_data" prelude), so this strengthens the regression guard without changing runtime behavior. Made-with: Cursor
…y-api-malformed-json-on-empty-aggregation
clickhouse-gh
Bot
added
pr-bugfix
Address clickhouse-gh AI Review on PR ClickHouse#103286 (❌ Blocker, src/Functions/TimeSeries/timeSeriesRange.cpp:280): guard i == 0 explicitly when computing values_base_offset. Note on correctness: IColumn::Offsets is a PaddedPODArray<UInt64> with reverse padding, so (*values_offsets)[-1] == 0 is well-defined. This is the canonical idiom used by ColumnArray::offsetAt(ssize_t i) and sizeAt(ssize_t i) in src/Columns/ColumnArray.h:235-236, and the existing 03254_timeseries_range stateless reference already exercises the empty-array first-row case (`SELECT timeSeriesFromGrid(..., []::Array(Float64))`) deterministically. The explicit `(i == 0) ? 0 : (*values_offsets)[i - 1]` does not change runtime behavior; it makes the intent obvious at the call site without requiring the reader to know the PaddedPODArray invariant, and removes the silent dependence on it. Made-with: Cursor
Contributor
LLVM Coverage Report
Changed lines: 70.49% (43/61) · Uncovered code |
vitlibar
reviewed
May 7, 2026
|
|
||
| The current timestamp is increased by `step` until it becomes greater than `end_timestamp` | ||
| If the number of the values doesn't match the number of the timestamps, the function throws an exception. | ||
| An empty input array `[]` is treated as "no samples for this row" and yields an empty result `[]`. |
Member
There was a problem hiding this comment.
No, this fix is wrong, I'll make my PR to address the issues.
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
2 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.
When running the prometheus/compliance test harness, the robots found a regression.
The fix itself is pretty simple, so the majority of the line change is comments on behavior and confirmation via tests.
Let me know if I need to slim down either.
The below is the robot's description of the behavior/tests/fixes. It's verbose but I like it, so I kept it.
Summary
The Prometheus query API endpoints (
/api/v1/query,/api/v1/query_range) returned an unbalanced JSON body when an aggregation over a non-existent metric reached thetimeSeriesFromGridstep function. Reproducer:returned HTTP 400 with body:
The outer
{, the data{, and theresult[are all left unclosed. The Prometheus Go client (v1.apiResponse.Data) errors withReadArrayCB: expect ] in the end, but found <EOF>. All five trivial aggregations (count/sum/avg/min/max) over a non-existent metric hit this, as does any case where the inner aggregation produces zero rows buttimeSeriesFromGridis still asked to emit N steps.Two cooperating defects, both fixed here:
1. Function-level (
src/Functions/TimeSeries/timeSeriesRange.cpp)timeSeriesFromGridthrewNumber of values (0) doesn't match number of steps (N)for any row whose values array was empty. The Prometheus contract forquery_rangeover an empty aggregation is an empty matrix series, not a hard error. The function now treatsnum_values == 0as "no samples for this row" and emits an empty per-row time series. The truly-mismatched case (0 < num_values != num_steps) still throwsBAD_ARGUMENTS, preserving the existing strict contract.The row-0 base offset is computed with an explicit
(i == 0) ? 0 : (*values_offsets)[i - 1]guard.IColumn::Offsetsis aPaddedPODArray<UInt64>with reverse padding, sooffsets[-1] == 0is well-defined (the canonical idiom inColumnArray::offsetAt/sizeAt); the explicit guard makes the row-0 case obvious at the call site without depending on that invariant.2. Handler-level (
src/Server/PrometheusRequestHandler.cpp,src/Storages/TimeSeries/PrometheusHTTPProtocolAPI.cpp)The PromQL HTTP handler streamed the success envelope (
{"status":"success","data":{"resultType":"matrix","result":[) directly into the response output stream, then on exception appended a second{"status":"error",...}object onto the still-open buffer — the unbalanced body above. The fix has two parts:PrometheusHTTPProtocolAPI::writeQueryResponseperforms the firstPullingPipelineExecutor::pull(...)before emitting any byte of the success envelope. The dominant PromQL evaluation failure mode (e.g.timeSeriesFromGridover an empty aggregation, malformed PromQL parse errors) raises on that first pull because the underlying pipeline is lazy, so it propagates with the response stream still untouched. Bulk row data continues to stream to the network (no in-memory accumulation, no peak-memory regression for largequery_rangeresponses).QueryAPIImpl::handlingRequestWithContextbranches its catch block onbody_buf.count() == 0:writeJSONString(it embeds SQL fragments that can contain quotes/backslashes which would otherwise produce another class of invalid JSON).PrometheusRequestHandler::handleRequestcatch invokesWriteBufferFromHTTPServerResponse::cancelWithException, which appends a__exception__marker block in a new chunk and intentionally skips the final empty chunk to break HTTP. The client sees a connection-level abort instead of concatenated JSON — wire format stays well-defined and a second JSON document is never written.Either fix on its own defends against the specific reproducer; both land because the function-level fix restores the regressed PASS for the specific aggregation queries, while the handler-level fix is defense-in-depth so any future PromQL function failure (early or late) cannot produce malformed JSON.
Tests
tests/queries/0_stateless/03254_timeseries_range.sqlgains three cases right after the existing wrong-countBAD_ARGUMENTSblock: emptyArray(Nullable(Float64)), emptyArray(Float64), and a multi-row case mixing empty and populated arrays viaarrayJointo prove an empty row does not poison sibling rows in the same block. The existingBAD_ARGUMENTScases are unchanged.tests/integration/test_prometheus_protocols/test_evaluation.pygains two complementary tests:test_query_range_empty_aggregation_returns_empty_matrix— strict regression guard for the function-level fix. Issuescount/sum/avg/min/max(nonexistent_metric_name)over a 15-minute / 15-second-step range and asserts HTTP 200 +status == "success"+ empty matrix. Cannot silently re-pass iftimeSeriesFromGridregresses to throwing.test_query_range_invalid_promql_returns_structured_error— companion guard for the handler's pre-output error path. Issues a syntactically broken PromQL expression and asserts HTTP400+status == "error"+errorType == "bad_data"+ presence of theerrorfield. Locks both the status code and envelope shape so a regression on either layer is caught.Verified end-to-end against a live ClickHouse instance: all five reproducer queries now return HTTP 200 with
{"status":"success","data":{"resultType":"matrix","result":[]}}; parse-time and other pre-stream error paths now return clean{"status":"error","errorType":"bad_data","error":"..."}envelopes with HTTP 400; late-stream failures abort the chunked transfer cleanly viacancelWithExceptionrather than appending a second JSON document.Made-with: Cursor
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix malformed JSON from
/api/v1/query_rangeon empty aggregations