From ec34b9fa3ae6a51e9170720aed01151b9bb9981f Mon Sep 17 00:00:00 2001 From: zz_y Date: Sun, 17 May 2026 17:27:31 -0600 Subject: [PATCH] fix(query): saturating_sub for time=0 + clean up dangling resolve_sketch_metric_alias callers (B6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes in `data_plane/src/query_engines/asap_query_engine/engine.rs`: 1. **`calculate_start_timestamp_promql` underflow.** The `OnlySpatial` branch did `end_timestamp - (scrape_interval * 1000)` with `end_timestamp: u64`. When the PromQL request has `time=0` (capability-miss feedback-loop probe shape, see `tests::capability_miss_http_e2e_tests`), `end_timestamp` is 0 and the bare subtraction underflows with `attempt to subtract with overflow`. Switch to `saturating_sub` — clamps to 0, which the downstream store query treats as a [0, 0]-width range. Empty result is the right answer for a probe looking for the capability-miss signal, not data. Currently dormant under the current call ordering (`process_via_simple_engine` calls legacy first, which short-circuits at the empty-streaming-config check before reaching line 611), but surfaces the moment the dispatch is reordered to modern-first. Fix removes the latent blocker for step 4 of #272. 2. **Delete dangling `resolve_sketch_metric_alias` callers at engine.rs:3024 and engine.rs:3525.** PR #275 retired the `resolve_sketch_metric_alias` method itself (the runtime refactor preserves metric names through the sketch processors — the suffix rewrite hadn't done anything in production for months). The PR removed the method definition + the legacy handle_query_promql call site but missed these two call sites inside `execute_range_promql_modern` and `QueryEngine::execute()` (both added by PR #274, which #275 was supposed to revert). Result: main fails to compile with two E0599s. This PR removes them inline as a fix-forward. Both call sites had the same shape: let query_owned = self .resolve_sketch_metric_alias(query) .unwrap_or_else(|| query.to_string()); let query = query_owned.as_str(); Removing the snippet leaves the original `query: &str` parameter in scope, which is what every downstream user wants — the analyzer accepts a `&str` directly. No semantic change beyond "don't try to rewrite a metric name that doesn't get rewritten by anyone." Test plan: * `cargo test -p data_plane --lib`: 749/749 pass (was 748; +1 for the new `calculate_start_timestamp_promql_handles_time_zero_without_underflow` regression test that pins the saturating_sub semantics). * Existing `capability_miss_http_e2e` tests still pass — the legacy path that previously triggered the underflow is still reached the same way (modern is still called second); the fix is dormant until the reorder lands but is now safe. Unblocks the next step of #272 — `process_via_simple_engine` modern-first reorder. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../query_engines/asap_query_engine/engine.rs | 60 +++++++++++-------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/data_plane/src/query_engines/asap_query_engine/engine.rs b/data_plane/src/query_engines/asap_query_engine/engine.rs index cd6ec848c..726f97410 100644 --- a/data_plane/src/query_engines/asap_query_engine/engine.rs +++ b/data_plane/src/query_engines/asap_query_engine/engine.rs @@ -608,7 +608,7 @@ impl ASAPQueryEngine { end_timestamp - (range_seconds * 1000) } QueryPatternType::OnlySpatial => { - end_timestamp - (self.prometheus_scrape_interval * 1000) + end_timestamp.saturating_sub(self.prometheus_scrape_interval * 1000) } } } @@ -3015,16 +3015,6 @@ impl ASAPQueryEngine { )); }; - // Schema-retirement #5 step 2: apply the same metric-rename - // rewrite the modern execute() instant path does, so range - // queries like `quantile_over_time(0.99, http_latency[5m])` - // bind to the suffixed series the agent's DDSketch processor - // emits. Mirrors the legacy `handle_query_promql` entry. - let query_owned = self - .resolve_sketch_metric_alias(query) - .unwrap_or_else(|| query.to_string()); - let query = query_owned.as_str(); - let analysis = control_plane::asap_tier_analysis::analyze_promql_for_asap_tier(query); @@ -3512,20 +3502,6 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu // over to archive (no per-candidate hybrid stitch yet — // that's the documented follow-up). if let Some(idx) = self.sketch_index.as_ref() { - // Schema-retirement #5 step 2: apply the agent-side - // INGEST-time metric-rename rewrite (DDSketch/KLL - // `_quantile`, HLL `_hll`) here at the top of modern - // execute() so bare-metric PromQL still hits the - // suffixed series the ASAP tier actually holds. The - // legacy `handle_query_promql` did this rewrite at - // its own entry; with the legacy path slated for - // retirement, the modern path needs the same - // capability so it can fully supersede. - let query_owned = self - .resolve_sketch_metric_alias(query) - .unwrap_or_else(|| query.to_string()); - let query = query_owned.as_str(); - let analysis = control_plane::asap_tier_analysis::analyze_promql_for_asap_tier(query); // Branch 1 — the control plane analyzer rejects the shape. @@ -6380,3 +6356,37 @@ mod analyzer_parity_tests { ); } } + +#[cfg(test)] +mod calculate_start_timestamp_promql_tests { + use super::*; + use crate::storage_engines::types::{HotReloadStreamingConfig, StreamingConfig}; + + #[test] + fn calculate_start_timestamp_promql_handles_time_zero_without_underflow() { + // Capability-miss probe queries fire with time=0 (Unix epoch). + // Pre-fix: u64 subtraction underflows and panics with + // "attempt to subtract with overflow". Post-fix: saturating_sub + // clamps to 0, which the downstream store query treats as a + // [0, 0]-width range — degenerates to an empty result, the + // right answer when the probe is looking for capability-miss + // signal not data. + + let hot_reload = HotReloadStreamingConfig::from_arc(Arc::new(StreamingConfig::default())); + let engine = ASAPQueryEngine::new_with_hot_reload(hot_reload, 15000); + + // Synthesize a minimal OnlySpatial match_result. The body of + // calculate_start_timestamp_promql for OnlySpatial only reads + // `self.prometheus_scrape_interval`; the match_result arg is + // unused in that branch. A default-constructed + // PromQLMatchResult is fine. + let mr = PromQLMatchResult::new(); + + let start = engine.calculate_start_timestamp_promql( + 0, // end_timestamp = 0 (the bug trigger) + QueryPatternType::OnlySpatial, + &mr, + ); + assert_eq!(start, 0, "saturating_sub should clamp to 0, not panic"); + } +}