From 5b273b71b89661e042c09a398bd9a1ce8e47f4d6 Mon Sep 17 00:00:00 2001 From: Jeff Larson Date: Tue, 28 Jul 2026 18:32:26 -0700 Subject: [PATCH 1/2] fix(server): cap query_metric_series window at retention, not a flat 90 days (JEF-593) query_metric_series resolved its window with a fixed 24*90-hour ceiling while metric_series_rollups is itself pruned at WATCHER_RETENTION_DAYS (default 7, same as the facet/histogram/hist_facet siblings' fixed 24*7). A wide request scanned an index/heap range that structurally holds no rows past retention -- wasted scan width and an inconsistent, misleading ceiling versus the sibling metric endpoints. Adds metric_series_max_hours(), sourced from WATCHER_RETENTION_DAYS (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 queries' fixed 24*7 both when the var is unset/unparsable and when retention is disabled (<= 0), since a disabled sweep prunes nothing and shouldn't be read as license for an unbounded scan. JEF-561's adaptive output bucketing is unchanged. Tests: parse_retention_max_hours unit tests cover the default-matches- sibling-ceiling case, tracking a non-default retention, and both disabled/invalid fallbacks; resolve_window_clamps_series_window_to_retention_ceiling proves a 90-day request is clamped to the 7-day retention bound (acceptance criterion). Closes JEF-593 Co-authored-by: Claude Sonnet 5 --- server/src/api.rs | 85 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 5 deletions(-) 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. From 39a4e1724d91e37fa49387ee652e5633b88e64c5 Mon Sep 17 00:00:00 2001 From: Jeff Larson Date: Tue, 28 Jul 2026 19:50:05 -0700 Subject: [PATCH 2/2] fix(server): rebase smoke tests' wide-window fixtures onto the new 7-day ceiling (JEF-593) metric_series_wide_window_returns_bounded_correct_points and metric_series_wide_window_reaggregates_merged_buckets_correctly hardcoded the old flat 90-day query_metric_series ceiling: seed offsets at 89d/45d/ 10d/91d, and an output-bucket WIDTH of 7800.0 (300 * ceil(2160h-in-secs / 300 / 1000)). Under the new retention-derived 7-day ceiling (metric_series_max_hours(), JEF-593) those fixtures fall outside the window entirely -- both tests would fail against a real Postgres (they silently no-op locally via pool_or_skip when DATABASE_URL is unset). Rebased both onto the 7-day ceiling, preserving the exact properties each pins: - returns_bounded_correct_points: seeds now sit at 6d/3d/1d (in-window) and 8d (just past the ceiling, must be excluded) -- same 1-day margins on each side of the boundary as the original 89d/91d pair. - reaggregates_merged_buckets_correctly: WIDTH recomputed for the 7-day (604800s) window -- 300 * ceil(604800 / 300 / 1000) = 900 -- with the bucket anchor moved from 5d to 3.5d ago (still well clear of both the "now" and ceiling edges) and the count-weighted-merge assertion (4.25) unchanged. Local cargo fmt/check/clippy are green; the smoke suite still skips locally (no Docker/Postgres in this environment) -- verified by reasoning through output_bucket_secs' bucket-floor math rather than by running against a live database. CI's server check (with Postgres) is the real validator for this commit. Co-authored-by: Claude Sonnet 5 --- server/tests/smoke.rs | 61 +++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 29 deletions(-) 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