Compare commits

...

1 Commits

Author SHA1 Message Date
Srikanth Chekuri
da9b4644df fix(prometheus): set NoStepSubqueryIntervalFn to stop promql subquery segfault (#12720)
Some checks are pending
build-staging / prepare (push) Waiting to run
build-staging / js-build (push) Blocked by required conditions
build-staging / go-build (push) Blocked by required conditions
build-staging / staging (push) Blocked by required conditions
cacheci / tests (push) Waiting to run
Release Drafter / update_release_draft (push) Waiting to run
#### 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)
2026-08-29 09:26:57 +00:00
6 changed files with 261 additions and 0 deletions

View File

@@ -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()
},
})
}

View File

@@ -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)
}

View File

@@ -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":{}}

View File

@@ -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"
}

View File

@@ -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,
)

View File

@@ -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]