diff --git a/server/src/api.rs b/server/src/api.rs index 07d716a..895ce0e 100644 --- a/server/src/api.rs +++ b/server/src/api.rs @@ -513,17 +513,42 @@ pub async fn metric_series( Ok(Json(query_metric_series(&pool, q).await.map_err(internal)?)) } +/// Hard ceiling (hours) on `query_metric_series`'s window, sourced from +/// `WATCHER_RETENTION_DAYS` (JEF-593) — the same knob `retention::prune_once` +/// prunes `metric_series_rollups` against — rather than a hard-wired constant, +/// so raising retention widens the queryable window without a code change. +/// Falls back to the sibling facet/histogram/hist_facet queries' fixed +/// `24 * 7` ceiling both when unset/unparsable and when retention is disabled +/// (`<= 0`), since a disabled sweep prunes nothing and shouldn't be read as an +/// unbounded scan window. +fn metric_series_max_hours() -> i32 { + parse_retention_max_hours(std::env::var("WATCHER_RETENTION_DAYS").ok().as_deref()) +} + +/// Pure helper behind `metric_series_max_hours()`, extracted so it can be +/// unit-tested without mutating process-global env state. +fn parse_retention_max_hours(raw: Option<&str>) -> i32 { + match raw.and_then(|s| s.parse::().ok()) { + Some(days) if days > 0 => days * 24, + _ => 24 * 7, + } +} + /// One metric's collapsed time series, shared by the HTTP handler and the MCP /// `metric_series` tool. Applies the same hours clamp, and honors an absolute -/// `from`/`to` window when given (see `SeriesQuery`). The 90-day ceiling here is -/// deliberate (JEF-561): the covering index (JEF-548) makes the read itself -/// cheap, and the adaptive output bucket below keeps the *result* bounded -/// regardless of window width, so there's no remaining cost to widening it. +/// `from`/`to` window when given (see `SeriesQuery`). The ceiling used to be a +/// flat 90 days on the theory that the covering index (JEF-548) makes the read +/// cheap and the adaptive output bucket below keeps the *result* bounded +/// regardless of window width — but `metric_series_rollups` is itself pruned at +/// `WATCHER_RETENTION_DAYS` (default 7), so anything past that structurally +/// holds no rows: widening the scan range there was pure waste. Capped to +/// `metric_series_max_hours()` instead (JEF-593), which tracks retention. pub async fn query_metric_series( pool: &PgPool, q: SeriesQuery, ) -> Result, sqlx::Error> { - let (lo, hi, width) = resolve_window_and_width(q.hours, 24, 24 * 90, q.from, q.to); + let (lo, hi, width) = + resolve_window_and_width(q.hours, 24, metric_series_max_hours(), q.from, q.to); sqlx::query_as::<_, SeriesPoint>( "SELECT metric_bucket(bucket, $5) AS t, sum(sum) / nullif(sum(count), 0) AS v FROM metric_series_rollups @@ -1466,6 +1491,56 @@ mod tests { assert_eq!(parse_max_lookback_hours(Some("48")), 48); } + // JEF-593: at the default 7-day retention, the series ceiling must match + // the sibling facet/histogram/hist_facet queries' fixed `24 * 7` — the old + // 90-day ceiling scanned 83 days of range that retention had already + // pruned to nothing. + #[test] + fn parse_retention_max_hours_matches_sibling_ceiling_at_default_retention() { + assert_eq!(parse_retention_max_hours(None), 24 * 7); + } + + #[test] + fn parse_retention_max_hours_tracks_a_non_default_retention() { + assert_eq!(parse_retention_max_hours(Some("3")), 24 * 3); + assert_eq!(parse_retention_max_hours(Some("30")), 24 * 30); + } + + // A disabled sweep (`<= 0`) prunes nothing, but must not be read as license + // to scan an unbounded range — fall back to the sibling ceiling instead. + #[test] + fn parse_retention_max_hours_falls_back_to_sibling_ceiling_when_retention_disabled() { + assert_eq!(parse_retention_max_hours(Some("0")), 24 * 7); + assert_eq!(parse_retention_max_hours(Some("-1")), 24 * 7); + } + + #[test] + fn parse_retention_max_hours_falls_back_to_sibling_ceiling_when_invalid() { + assert_eq!(parse_retention_max_hours(Some("not a number")), 24 * 7); + } + + // Acceptance criterion: a >retention window request is clamped to the + // retention bound, exactly like JEF-532's absolute-window clamp test above + // — but driven by the retention-derived ceiling instead of a literal. + #[test] + fn resolve_window_clamps_series_window_to_retention_ceiling() { + let hi = Utc::now(); + let from = hi - Duration::days(90); + let (lo, resolved_hi) = resolve_window( + None, + 24, + parse_retention_max_hours(None), + Some(from), + Some(hi), + ); + assert_eq!(resolved_hi, hi); + assert_eq!( + lo, + hi - Duration::hours(24 * 7), + "a 90-day span should be clamped down to the 7-day retention ceiling" + ); + } + // JEF-561: a narrow window (well under TARGET_POINTS * base) must collapse // to exactly `base` — unwidened, full rollup resolution — so a short-range // chart's points are unaffected. diff --git a/server/tests/smoke.rs b/server/tests/smoke.rs index e9ca31a..f7bc7a1 100644 --- a/server/tests/smoke.rs +++ b/server/tests/smoke.rs @@ -740,16 +740,17 @@ async fn metric_series_honors_absolute_from_to_window() { assert_eq!(buckets[0]["counts"], serde_json::json!([0, 5, 0])); } -/// The series endpoint's window can be up to 90 days wide (`resolve_window`'s -/// `max_hours` for `/api/metrics/series`, JEF-548) — this is the query whose -/// wide-window rollup scan was traced holding a pool connection for tens of -/// seconds (the fix: a covering index on `metric_series_rollups`, migration -/// 0017). This doesn't benchmark the index directly (no way to force a plan -/// choice through the HTTP API), but it does pin the behavior the index must -/// not change: a request at the 90-day ceiling returns exactly the points -/// inside that window — none dropped, none leaked in from just outside it — -/// so a future change to the index or the query can't silently narrow or -/// widen what's returned. +/// The series endpoint's window is capped at the retention-derived ceiling +/// (`resolve_window`'s `max_hours` for `/api/metrics/series` — `24 * 7` at the +/// default `WATCHER_RETENTION_DAYS`, JEF-593; previously a flat 90 days, +/// JEF-548). Originally the query whose wide-window rollup scan was traced +/// holding a pool connection for tens of seconds (the fix: a covering index on +/// `metric_series_rollups`, migration 0017). This doesn't benchmark the index +/// directly (no way to force a plan choice through the HTTP API), but it does +/// pin the behavior the index must not change: a request at (or past) the +/// 7-day ceiling returns exactly the points inside that window — none +/// dropped, none leaked in from just outside it — so a future change to the +/// index or the query can't silently narrow or widen what's returned. #[tokio::test] #[serial] async fn metric_series_wide_window_returns_bounded_correct_points() { @@ -758,13 +759,14 @@ async fn metric_series_wide_window_returns_bounded_correct_points() { return; }; let day = 86_400.0; - insert_rollup_at(&pool, "wide.metric", 89.0 * day).await; - insert_rollup_at(&pool, "wide.metric", 45.0 * day).await; - insert_rollup_at(&pool, "wide.metric", 10.0 * day).await; - // Just past the 90-day ceiling: must be excluded, not silently included. - insert_rollup_at(&pool, "wide.metric", 91.0 * day).await; + insert_rollup_at(&pool, "wide.metric", 6.0 * day).await; + insert_rollup_at(&pool, "wide.metric", 3.0 * day).await; + insert_rollup_at(&pool, "wide.metric", 1.0 * day).await; + // Just past the 7-day ceiling: must be excluded, not silently included. + insert_rollup_at(&pool, "wide.metric", 8.0 * day).await; let router = app(pool); + // Requests far past the ceiling (2160h = 90d) must still clamp to it. let (status, series) = get_json(&router, "/api/metrics/series?name=wide.metric&hours=2160").await; assert_eq!(status, StatusCode::OK); @@ -772,26 +774,27 @@ async fn metric_series_wide_window_returns_bounded_correct_points() { assert_eq!( arr.len(), 3, - "the 91-day-old point is outside the 90-day window, series = {arr:?}" + "the 8-day-old point is outside the 7-day window, series = {arr:?}" ); for point in arr { assert_eq!(point["v"], 1.0); } - // Ascending by time — oldest (89d) first, newest (10d) last. + // Ascending by time — oldest (6d) first, newest (1d) last. let times: Vec<&str> = arr.iter().map(|p| p["t"].as_str().unwrap()).collect(); let mut sorted = times.clone(); sorted.sort(); assert_eq!(times, sorted, "points must be ordered t ASC"); } -/// JEF-561: at the 90-day ceiling the adaptive output bucket widens from the -/// base 300s rollup bucket to `300 * ceil((2160h in secs / 300) / 1000) = 7800s` -/// (see `output_bucket_secs`'s unit tests for that math) — a chart gets ~1000 -/// points instead of ~26k. This pins that the widening re-aggregates rollup -/// rows *correctly*: two rows placed well inside the same 7800s output bucket -/// must collapse into one point whose value is the count-weighted average of -/// both (not a plain average), while a row a full bucket-width away stays its -/// own point, unmerged. +/// JEF-561/JEF-593: at the (now retention-derived) 7-day ceiling the adaptive +/// output bucket widens from the base 300s rollup bucket to +/// `300 * ceil((168h in secs / 300) / 1000) = 900s` (see `output_bucket_secs`'s +/// unit tests for that math) — a chart gets bounded output instead of one +/// point per raw rollup bucket. This pins that the widening re-aggregates +/// rollup rows *correctly*: two rows placed well inside the same 900s output +/// bucket must collapse into one point whose value is the count-weighted +/// average of both (not a plain average), while a row a full bucket-width +/// away stays its own point, unmerged. #[tokio::test] #[serial] async fn metric_series_wide_window_reaggregates_merged_buckets_correctly() { @@ -799,11 +802,11 @@ async fn metric_series_wide_window_reaggregates_merged_buckets_correctly() { eprintln!("skipping: DATABASE_URL not set"); return; }; - const WIDTH: f64 = 7800.0; + const WIDTH: f64 = 900.0; let now_epoch = chrono::Utc::now().timestamp() as f64; - // A bucket safely inside the 90-day window (well clear of "now" and of the - // 91-day exclusion edge exercised by the sibling test above). - let bucket_start = ((now_epoch - 5.0 * 86_400.0) / WIDTH).floor() * WIDTH; + // A bucket safely inside the 7-day window (well clear of "now" and of the + // ceiling edge exercised by the sibling test above). + let bucket_start = ((now_epoch - 3.5 * 86_400.0) / WIDTH).floor() * WIDTH; let secs_ago_at = |frac: f64| now_epoch - (bucket_start + WIDTH * frac); // Two rows well inside the same output bucket (30%/60% across it — a wide