From da9b4644dfc26e4bf81658edd3bc015df8897c4a Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Sat, 29 Aug 2026 09:26:57 +0000 Subject: [PATCH] fix(prometheus): set NoStepSubqueryIntervalFn to stop promql subquery segfault (#12720) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Description - A PromQL subquery without a step, for example `max_over_time(metric[5m:])`, segfaulted the whole query-service. The engine calls `NoStepSubqueryIntervalFn` for such subqueries, and we build the engine without it, so the call hits a nil function. - The bug is present on every PromQL surface, because all of them share the one engine constructor in `pkg/prometheus/engine.go`: v3 and v5 `query_range`, `/api/v1/query`, the clickhousev2 transpiler, and promql alert rules. A saved rule with such a subquery crash-loops the instance on its own schedule. - The fix sets the callback to 1m. This matches the Prometheus default global `evaluation_interval`, which upstream wires into this field. One place fixes every path. - This is the root cause of the SigNoz/platform-pod#3068 incident. The instance-hardening request from that incident is tracked in SigNoz/pulse-pod#308. #### Issues closed by this PR Closes SigNoz/platform-pod#3068 #### Additional Information We audited `EngineOpts` for more bugs of the same class. `NoStepSubqueryIntervalFn` is the only field the engine calls without a nil guard; `promql.NewEngine` defaults the other nil-able fields (`Parser`, `FeatureRegistry`). The remaining gaps against upstream wiring are not crashes, and we filed them separately: SigNoz/pulse-pod#305 (`@` modifier and negative offset disabled), SigNoz/pulse-pod#306 (engine self-metrics not registered), SigNoz/pulse-pod#307 (active query tracker startup panic risk), SigNoz/pulse-pod#309 (step guard in the v3 cache), SigNoz/pulse-pod#310 (upstream proposal to fail fast on the nil callback). Tests for the bug: - `pkg/prometheus/engine_test.go` — fails with the exact segfault when the fix is removed. - `tests/integration/tests/promqlconformance/04_no_step_subquery.py` — a step-less subquery through `/api/v5/query_range` returns correct values on both providers, and the service stays up. - `tests/integration/tests/alerts/04_promql_subquery_no_step.py` — a promql alert rule with a step-less subquery evaluates and fires. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- pkg/prometheus/engine.go | 7 ++ pkg/prometheus/engine_test.go | 33 +++++++ .../promql_subquery_no_step/alert_data.jsonl | 5 + .../promql_subquery_no_step/rule.json | 58 ++++++++++++ .../alerts/04_promql_subquery_no_step.py | 93 +++++++++++++++++++ .../promqlconformance/04_no_step_subquery.py | 65 +++++++++++++ 6 files changed, 261 insertions(+) create mode 100644 pkg/prometheus/engine_test.go create mode 100644 tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/alert_data.jsonl create mode 100644 tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/rule.json create mode 100644 tests/integration/tests/alerts/04_promql_subquery_no_step.py create mode 100644 tests/integration/tests/promqlconformance/04_no_step_subquery.py diff --git a/pkg/prometheus/engine.go b/pkg/prometheus/engine.go index b0984d627b7..3469615274a 100644 --- a/pkg/prometheus/engine.go +++ b/pkg/prometheus/engine.go @@ -2,6 +2,7 @@ package prometheus import ( "log/slog" + "time" "github.com/prometheus/prometheus/promql" ) @@ -23,5 +24,11 @@ func NewEngine(logger *slog.Logger, cfg Config) *Engine { Timeout: cfg.Timeout, ActiveQueryTracker: activeQueryTracker, LookbackDelta: cfg.LookbackDelta, + // The engine calls this for subqueries that do not set a step, such as + // `metric[5m:]`, and segfaults if it is nil. 1m matches the default + // global evaluation_interval that Prometheus wires here. + NoStepSubqueryIntervalFn: func(int64) int64 { + return time.Minute.Milliseconds() + }, }) } diff --git a/pkg/prometheus/engine_test.go b/pkg/prometheus/engine_test.go new file mode 100644 index 00000000000..f8003fc5754 --- /dev/null +++ b/pkg/prometheus/engine_test.go @@ -0,0 +1,33 @@ +package prometheus + +import ( + "context" + "log/slog" + "testing" + "time" + + "github.com/prometheus/prometheus/storage" + "github.com/stretchr/testify/require" +) + +func TestNoStepSubqueryDoesNotPanic(t *testing.T) { + engine := NewEngine(slog.New(slog.DiscardHandler), Config{Timeout: time.Minute}) + queryable := storage.QueryableFunc(func(int64, int64) (storage.Querier, error) { + return storage.NoopQuerier(), nil + }) + + qry, err := engine.NewRangeQuery( + context.Background(), + queryable, + nil, + "max_over_time(some_metric[5m:])", + time.Now().Add(-time.Hour), + time.Now(), + time.Minute, + ) + require.NoError(t, err) + defer qry.Close() + + res := qry.Exec(context.Background()) + require.NoError(t, res.Err) +} diff --git a/tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/alert_data.jsonl b/tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/alert_data.jsonl new file mode 100644 index 00000000000..9b4cf777f57 --- /dev/null +++ b/tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/alert_data.jsonl @@ -0,0 +1,5 @@ +{"metric_name":"cpu_percent_promql_subquery_no_step","labels":{"host":"server-01","cpu":"cpu0"},"timestamp":"2026-01-29T10:01:00+00:00","value":15,"temporality":"Unspecified","type_":"Gauge","is_monotonic":false,"flags":0,"description":"","unit":"","env":"default","resource_attrs":{},"scope_attrs":{}} +{"metric_name":"cpu_percent_promql_subquery_no_step","labels":{"host":"server-01","cpu":"cpu0"},"timestamp":"2026-01-29T10:02:00+00:00","value":15,"temporality":"Unspecified","type_":"Gauge","is_monotonic":false,"flags":0,"description":"","unit":"","env":"default","resource_attrs":{},"scope_attrs":{}} +{"metric_name":"cpu_percent_promql_subquery_no_step","labels":{"host":"server-01","cpu":"cpu0"},"timestamp":"2026-01-29T10:03:00+00:00","value":15,"temporality":"Unspecified","type_":"Gauge","is_monotonic":false,"flags":0,"description":"","unit":"","env":"default","resource_attrs":{},"scope_attrs":{}} +{"metric_name":"cpu_percent_promql_subquery_no_step","labels":{"host":"server-01","cpu":"cpu0"},"timestamp":"2026-01-29T10:04:00+00:00","value":15,"temporality":"Unspecified","type_":"Gauge","is_monotonic":false,"flags":0,"description":"","unit":"","env":"default","resource_attrs":{},"scope_attrs":{}} +{"metric_name":"cpu_percent_promql_subquery_no_step","labels":{"host":"server-01","cpu":"cpu0"},"timestamp":"2026-01-29T10:05:00+00:00","value":15,"temporality":"Unspecified","type_":"Gauge","is_monotonic":false,"flags":0,"description":"","unit":"","env":"default","resource_attrs":{},"scope_attrs":{}} diff --git a/tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/rule.json b/tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/rule.json new file mode 100644 index 00000000000..72c6f7c4b75 --- /dev/null +++ b/tests/integration/testdata/alerts/test_scenarios/promql_subquery_no_step/rule.json @@ -0,0 +1,58 @@ +{ + "alert": "promql_subquery_no_step", + "ruleType": "promql_rule", + "alertType": "METRIC_BASED_ALERT", + "condition": { + "thresholds": { + "kind": "basic", + "spec": [ + { + "name": "critical", + "target": 10, + "matchType": "at_least_once", + "op": "above", + "channels": [ + "test channel" + ] + } + ] + }, + "compositeQuery": { + "queryType": "promql", + "panelType": "graph", + "queries": [ + { + "type": "promql", + "spec": { + "name": "A", + "query": "max_over_time({\"cpu_percent_promql_subquery_no_step\"}[2m:])" + } + } + ] + }, + "selectedQueryName": "A" + }, + "evaluation": { + "kind": "rolling", + "spec": { + "evalWindow": "5m0s", + "frequency": "15s" + } + }, + "labels": {}, + "annotations": { + "description": "This alert is fired when the defined metric (current value: {{$value}}) crosses the threshold ({{$threshold}})", + "summary": "This alert is fired when the defined metric (current value: {{$value}}) crosses the threshold ({{$threshold}})" + }, + "notificationSettings": { + "groupBy": [], + "usePolicy": false, + "renotify": { + "enabled": false, + "interval": "30m", + "alertStates": [] + } + }, + "version": "v5", + "schemaVersion": "v2alpha1" +} diff --git a/tests/integration/tests/alerts/04_promql_subquery_no_step.py b/tests/integration/tests/alerts/04_promql_subquery_no_step.py new file mode 100644 index 00000000000..3ea2292432d --- /dev/null +++ b/tests/integration/tests/alerts/04_promql_subquery_no_step.py @@ -0,0 +1,93 @@ +import json +import uuid +from collections.abc import Callable +from datetime import UTC, datetime, timedelta + +from wiremock.client import HttpMethods, Mapping, MappingRequest, MappingResponse + +from fixtures import types +from fixtures.alerts import ( + update_rule_channel_name, + verify_webhook_alert_expectation, +) +from fixtures.fs import get_testdata_file_path + +TEST_CASE = types.AlertTestCase( + name="promql_subquery_no_step", + rule_path="alerts/test_scenarios/promql_subquery_no_step/rule.json", + alert_data=[ + types.AlertData( + type="metrics", + data_path="alerts/test_scenarios/promql_subquery_no_step/alert_data.jsonl", + ), + ], + alert_expectation=types.AlertExpectation( + should_alert=True, + wait_time_seconds=30, + expected_alerts=[ + types.FiringAlert( + labels={ + "alertname": "promql_subquery_no_step", + "threshold.name": "critical", + } + ), + ], + ), +) + + +def test_promql_rule_subquery_without_step( + notification_channel: types.TestContainerDocker, + make_http_mocks: Callable[[types.TestContainerDocker, list[Mapping]], None], + create_webhook_notification_channel: Callable[[str, str, dict, bool], str], + create_alert_rule: Callable[[dict], str], + insert_alert_data: Callable[[list[types.AlertData], datetime], None], +): + """ + A promql rule with a step-less subquery ([2m:]) must evaluate and fire. + A nil NoStepSubqueryIntervalFn segfaults the process on first evaluation. + """ + notification_channel_name = str(uuid.uuid4()) + webhook_endpoint_path = f"/alert/{notification_channel_name}" + notification_url = notification_channel.container_configs["8080"].get(webhook_endpoint_path) + + make_http_mocks( + notification_channel, + [ + Mapping( + request=MappingRequest( + method=HttpMethods.POST, + url=webhook_endpoint_path, + ), + response=MappingResponse( + status=200, + json_body={}, + ), + persistent=False, + ) + ], + ) + + create_webhook_notification_channel( + channel_name=notification_channel_name, + webhook_url=notification_url, + http_config={}, + send_resolved=False, + ) + + insert_alert_data( + TEST_CASE.alert_data, + base_time=datetime.now(tz=UTC) - timedelta(minutes=5), + ) + + rule_path = get_testdata_file_path(TEST_CASE.rule_path) + with open(rule_path, encoding="utf-8") as f: + rule_data = json.loads(f.read()) + update_rule_channel_name(rule_data, notification_channel_name) + create_alert_rule(rule_data) + + verify_webhook_alert_expectation( + notification_channel, + notification_channel_name, + TEST_CASE.alert_expectation, + ) diff --git a/tests/integration/tests/promqlconformance/04_no_step_subquery.py b/tests/integration/tests/promqlconformance/04_no_step_subquery.py new file mode 100644 index 00000000000..ebca1f9a661 --- /dev/null +++ b/tests/integration/tests/promqlconformance/04_no_step_subquery.py @@ -0,0 +1,65 @@ +from collections.abc import Callable +from datetime import UTC, datetime, timedelta +from http import HTTPStatus +from uuid import uuid4 + +from fixtures import types +from fixtures.auth import USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD +from fixtures.metrics import Metrics +from fixtures.querier import get_all_series, make_query_request + +MINUTE_MS = 60_000 + +LEGS: list[tuple[str, dict | None]] = [ + ("default", None), + ("clickhousev2", {"X-SigNoz-PromQL-Provider": "clickhousev2"}), +] + + +def test_promql_subquery_without_step_evaluates( + signoz: types.SigNoz, + create_user_admin: None, # pylint: disable=unused-argument + get_token: Callable[[str, str], str], + insert_metrics: Callable[[list[Metrics]], None], +) -> None: + """ + A subquery that omits its step, e.g. `metric[5m:]`, is valid PromQL: the + engine fills in its default resolution. A nil NoStepSubqueryIntervalFn + segfaults the whole process on the first such query. + """ + end_ms = (int((datetime.now(tz=UTC) - timedelta(minutes=5)).timestamp() * 1000) // MINUTE_MS) * MINUTE_MS + start_ms = end_ms - 30 * MINUTE_MS + + metric = f"no_step_subquery_gauge_{uuid4().hex[:8]}" + insert_metrics( + [ + Metrics( + metric_name=metric, + labels={"host": "server-01"}, + timestamp=datetime.fromtimestamp(ts_ms / 1000, tz=UTC), + value=42.0, + ) + for ts_ms in range(start_ms, end_ms + 1, MINUTE_MS) + ] + ) + + token = get_token(USER_ADMIN_EMAIL, USER_ADMIN_PASSWORD) + + for leg, headers in LEGS: + query = {"type": "promql", "spec": {"name": "A", "query": f"max_over_time({metric}[5m:])"}} + response = make_query_request(signoz, token, start_ms, end_ms, [query], headers=headers) + assert response.status_code == HTTPStatus.OK, f"{leg}: {response.text[:300]}" + series = get_all_series(response.json(), "A") + assert series, f"{leg}: the subquery must return the inserted series" + values = {point["value"] for entry in series for point in entry.get("values") or []} + assert values == {42.0}, f"{leg}: {sorted(values)[:5]}" + + # A plain follow-up query proves the process survived the subquery legs. + response = make_query_request( + signoz, + token, + start_ms, + end_ms, + [{"type": "promql", "spec": {"name": "A", "query": metric}}], + ) + assert response.status_code == HTTPStatus.OK, response.text[:300]