fix(query): honor counter-function semantics in ExactAgg dispatch (closes #301, #300)

control_plane/src/asap_tier_analysis.rs

@@ -371,12 +371,14 @@ struct PromqlTrace {
     function: String,
     function_args: Vec<f64>,
     range_seconds: u64,
-    /// Set to `OuterFn::Rate` when ANY `rate(...)` or `irate(...)`
-    /// call is found anywhere in the expression tree; otherwise
-    /// `OuterFn::Plain`. The flag-style detection mirrors what the
-    /// retired `query_contains_rate_call` engine helper used to do
-    /// over the raw query string — done here once so the engine reads
-    /// it off the typed candidate.
+    /// Counter-function flavour recovered from the expression tree
+    /// (issue #301): `Rate` for `rate`/`irate`, `Increase` for
+    /// `increase`, `SumOverTime` for `sum_over_time`, else `Plain`
+    /// (bare selector / instant `sum`). The most-specific counter idiom
+    /// found anywhere in the tree wins (see [`set_counter_fn`]) so
+    /// composed shapes like `sum by (..) (rate(..))` report `Rate`.
+    /// Done here once so the engine reads it off the typed candidate
+    /// instead of re-parsing the raw query string.
     outer_fn: OuterFn,
     /// PromQL outer-aggregation operator wrapping the inner function —
     /// `max`/`min`/`avg`/`count`/`group`/`stddev`/`stdvar` only. `sum`
@@ -488,21 +490,49 @@ fn extract_outer_agg(expr: &Expr) -> OuterAgg {
     }
 }
 
+/// Set `t.outer_fn` honoring counter-idiom precedence (issue #301):
+/// `Rate` > `Increase` > `SumOverTime` > `Plain`. The walker may visit
+/// nested calls in any order, so a more-specific flavour already set
+/// must not be downgraded by a less-specific one seen later. (In
+/// practice a single counter query has exactly one of these, but
+/// pathological compositions like `increase(sum_over_time(...))` resolve
+/// deterministically.)
+fn set_counter_fn(t: &mut PromqlTrace, candidate: OuterFn) {
+    fn rank(f: OuterFn) -> u8 {
+        match f {
+            OuterFn::Rate => 3,
+            OuterFn::Increase => 2,
+            OuterFn::SumOverTime => 1,
+            OuterFn::Plain => 0,
+        }
+    }
+    if rank(candidate) > rank(t.outer_fn) {
+        t.outer_fn = candidate;
+    }
+}
+
 fn walk_ast_for_trace(expr: &Expr, t: &mut PromqlTrace) {
     match expr {
         Expr::Call(call) => {
             let name = call.func.name.to_lowercase();
             if t.function.is_empty() {
                 t.function = name.clone();
             }
-            // Flag `rate(...)` / `irate(...)` ANYWHERE in the tree —
-            // mirrors the retired `query_contains_rate_call` walker.
-            // For composed shapes like `sum by (zone) (rate(metric[r]))`
-            // the FIRST function set above is `"sum"` (the outer
-            // Aggregate), but `outer_fn` must still report `Rate` so
-            // the engine dispatches through `evaluate_exact_agg_rate`.
-            if matches!(name.as_str(), "rate" | "irate") {
-                t.outer_fn = OuterFn::Rate;
+            // Flag the counter-function flavour ANYWHERE in the tree
+            // (issue #301) — mirrors the retired `query_contains_rate_call`
+            // walker but with the full taxonomy. For composed shapes like
+            // `sum by (zone) (rate(metric[r]))` the FIRST function set
+            // above is `"sum"` (the outer Aggregate), but `outer_fn` must
+            // report the INNER counter function so the engine dispatches
+            // correctly. `rate`/`irate` win over `increase`, which wins
+            // over `sum_over_time` (most-specific-counter-idiom wins);
+            // `set_counter_fn` enforces that precedence so the order in
+            // which the walker encounters nested calls doesn't matter.
+            match name.as_str() {
+                "rate" | "irate" => set_counter_fn(t, OuterFn::Rate),
+                "increase" => set_counter_fn(t, OuterFn::Increase),
+                "sum_over_time" => set_counter_fn(t, OuterFn::SumOverTime),
+                _ => {}
             }
             for a in &call.args.args {
                 if let Expr::NumberLiteral(nl) = a.as_ref() {
@@ -968,19 +998,49 @@ mod tests {
     }
 
     #[test]
-    fn sum_over_time_candidate_carries_outer_fn_plain() {
+    fn sum_over_time_candidate_carries_outer_fn_sum_over_time() {
         // `sum_over_time(metric[r])` shares `Capability::ExactAgg(Sum)`
         // with `rate(metric[r])` — the capability alone can't
-        // disambiguate. The `outer_fn` field MUST report `Plain` so
-        // the engine takes the per-window reducer (no rate divisor).
+        // disambiguate. Post-#301 the `outer_fn` field reports
+        // `SumOverTime` so the engine can capability-miss → archive
+        // (asap can't reconstruct Σ-of-cumulative-samples from deltas).
         let a = analyze_promql_for_asap_tier("sum_over_time(http_requests_total[5m])");
         assert!(a.unsupported.is_none(), "{a:?}");
         assert_eq!(
             a.candidates[0].required_capability,
             Capability::ExactAgg(AggregationType::Sum),
             "{a:?}"
         );
-        assert_eq!(a.candidates[0].outer_fn, OuterFn::Plain, "{a:?}");
+        assert_eq!(a.candidates[0].outer_fn, OuterFn::SumOverTime, "{a:?}");
+    }
+
+    #[test]
+    fn increase_candidate_carries_outer_fn_increase() {
+        // `increase(metric[r])` shares `Capability::ExactAgg(Sum)` with
+        // `rate`/`sum_over_time`; the `outer_fn` field carries the
+        // distinction so the engine sums deltas in `[t-r,t]` WITHOUT the
+        // rate divisor (issue #301).
+        let a = analyze_promql_for_asap_tier("increase(http_requests_total[5m])");
+        assert!(a.unsupported.is_none(), "{a:?}");
+        assert_eq!(
+            a.candidates[0].required_capability,
+            Capability::ExactAgg(AggregationType::Sum),
+            "{a:?}"
+        );
+        assert_eq!(a.candidates[0].outer_fn, OuterFn::Increase, "{a:?}");
+    }
+
+    #[test]
+    fn sum_by_over_increase_candidate_carries_outer_fn_increase() {
+        // Composed `sum by (zone) (increase(metric[r]))` — inner counter
+        // function wins over the outer `sum` (same precedence as the
+        // rate case).
+        let a = analyze_promql_for_asap_tier(
+            "sum by (zone) (increase(http_requests_total[5m]))",
+        );
+        assert!(a.unsupported.is_none(), "{a:?}");
+        assert_eq!(a.candidates[0].outer_fn, OuterFn::Increase, "{a:?}");
+        assert_eq!(a.candidates[0].range_seconds, 300, "{a:?}");
     }
 
     #[test]
@@ -1042,7 +1102,7 @@ mod tests {
              can dispatch correctly without re-parsing the raw PromQL"
         );
         assert_eq!(rate.candidates[0].outer_fn, OuterFn::Rate);
-        assert_eq!(sot.candidates[0].outer_fn, OuterFn::Plain);
+        assert_eq!(sot.candidates[0].outer_fn, OuterFn::SumOverTime);
     }
 
     // ── outer_agg — outer aggregation operator on function results ──────

control_plane/src/sketch_algebra/capability.rs

@@ -127,28 +127,63 @@ pub enum Capability {
 /// helper to recover the distinction; that was a lossy-lowering smell.
 ///
 /// The walker that populates this lives in `asap_tier_analysis.rs`
-/// (`trace_from_promql`) — it sets `Rate` if ANY `rate(...)` or
-/// `irate(...)` Call appears anywhere in the expression tree, otherwise
-/// `Plain`. The taxonomy is intentionally minimal: today the engine
-/// only branches on "needs rate divisor or not". Future shape-specific
-/// dispatch (e.g. separating `increase` from `sum`) can extend this
-/// enum without touching the `Capability` algebra.
+/// (`trace_from_promql`) — it picks the most-specific counter-function
+/// flavour found anywhere in the expression tree (inner-function wins
+/// for composed shapes like `sum by (...) (rate(...))`).
+///
+/// ## Counter-function taxonomy (issue #301)
+///
+/// Post-#299 the agent streams per-window DELTAS for counters. The four
+/// PromQL counter idioms have genuinely different semantics over those
+/// deltas, but they ALL lower to a single `Capability::ExactAgg(Sum)`
+/// (the `AggIntent::Sum` collapse erases the function name). Before
+/// #301 the engine only distinguished `Rate` from everything else, so
+/// `sum`, `sum_over_time`, `increase`, and instant-sum all hit the same
+/// reducer path and returned the same (wrong) number. This enum carries
+/// the function distinction the engine needs to dispatch correctly:
+///
+/// | Variant       | PromQL                       | Engine dispatch                                   |
+/// |---------------|------------------------------|---------------------------------------------------|
+/// | `Plain`       | `sum(c)` / `sum by (..) (c)` | accumulate ALL windows → cumulative-since-storage |
+/// | `Rate`        | `rate(c[r])` / `irate(c[r])` | Σ deltas in `[t-r,t]` ÷ min(r, coverage)          |
+/// | `Increase`    | `increase(c[r])`             | Σ deltas in `[t-r,t]` (one cumulative number)     |
+/// | `SumOverTime` | `sum_over_time(c[r])`        | capability-miss → archive (can't reconstruct)     |
+///
+/// The taxonomy lives on `OuterFn` (not the `Capability` algebra) so the
+/// sid-matching half stays a pure `ExactAgg(Sum)` predicate — the
+/// function distinction is a query-evaluation concern, not a stored-state
+/// one.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)]
 pub enum OuterFn {
-    /// No rate-style outer function in the expression — bare selector,
-    /// `sum(metric)`, `sum by (...) (metric)`, `sum_over_time(metric[r])`,
-    /// `increase(metric[r])`, `count_over_time(metric[r])`, etc. The
-    /// engine dispatches to the plain per-window reducer. This is the
-    /// default — `Default::default()` returns `Plain` so candidates
-    /// built without an explicit outer-fn (test fixtures, fallback
-    /// paths) get the safe non-rate dispatch.
+    /// No range-style counter function in the expression — bare selector,
+    /// `sum(metric)`, `sum by (...) (metric)`. PromQL semantics for an
+    /// instant `sum` over a counter is "current cumulative counter value,
+    /// summed per group". Over per-window deltas the engine accumulates
+    /// EVERY window in storage up to `now` into one cumulative number per
+    /// group. This is the default — `Default::default()` returns `Plain`
+    /// so candidates built without an explicit outer-fn (test fixtures,
+    /// fallback paths) get the safe accumulate-all dispatch.
     #[default]
     Plain,
     /// `rate(metric[r])` or `irate(metric[r])` appears in the expression
-    /// (possibly nested inside an outer `sum by (...) (...)`). The
-    /// engine dispatches to `evaluate_exact_agg_rate`, which divides by
-    /// the range to produce events-per-second.
+    /// (possibly nested inside an outer `sum by (...) (...)`). The engine
+    /// dispatches to `evaluate_exact_agg_rate`, which sums the deltas in
+    /// `[t-r, t]` and divides by `min(r, actual_coverage_seconds)` to
+    /// produce events-per-second.
     Rate,
+    /// `increase(metric[r])` appears in the expression. PromQL semantics:
+    /// `counter(t) − counter(t−r)`. Over per-window deltas that is exactly
+    /// the sum of deltas in `[t-r, t]`. The engine dispatches to the
+    /// accumulate-across-windows path scoped to the `[t-r, t]` clip,
+    /// yielding ONE cumulative number per group (no `÷ r`).
+    Increase,
+    /// `sum_over_time(metric[r])` appears in the expression. PromQL
+    /// semantics: Σ of the (cumulative) SAMPLE values in `[r]` — a
+    /// quadratic over the storage horizon that asap CANNOT reconstruct
+    /// from stored deltas. The engine returns a capability-miss so the
+    /// query routes to the archive tier (which has raw samples) rather
+    /// than fabricating a wrong number. See issue #301 decision (a).
+    SumOverTime,
 }
 
 /// PromQL outer-aggregation operator carried on each `ASAPTierCandidate`