From cc70c06c3c030119465584308def3610a2fbb707 Mon Sep 17 00:00:00 2001 From: Mal Detair Date: Sat, 11 Jul 2026 01:17:35 +0200 Subject: [PATCH 1/2] fix(observability): repair command-center trend rollup refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `telemetry_trend_rollups` materialized view backing the admin Command Center's 7d/30d trend charts could never be refreshed. Its unique index was built on an expression (`COALESCE(route, '')`), and PostgreSQL only accepts a plain-column unique index as the row-matching index for `REFRESH ... CONCURRENTLY`. So the hourly refresh failed every cycle with "cannot refresh materialized view concurrently", the view stayed empty since introduction, and 7d/30d charts showed no data. Live (1h/6h/24h) ranges were unaffected — they read `telemetry_metric_samples` directly. - Migration recreates the view with `route` as a non-null column (`COALESCE(labels->>'http.route', '')`) and a plain-column unique index on `(day, metric_name, scope, route)`, which CONCURRENTLY accepts. - The refresh job now falls back to a blocking `REFRESH` if the concurrent refresh can't proceed, so rollups self-heal instead of silently going stale. Verified against the production schema/data in a rolled-back transaction: the recreated view populates (222 rows) and the index is plain-column. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JDEzVG39gGZFozPCndbSid --- CHANGELOG.md | 1 + ...000000_fix_command_center_rollup_index.sql | 38 +++++++++++++++++++ server/src/observability/retention.rs | 28 +++++++++++++- 3 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 server/migrations/20260711000000_fix_command_center_rollup_index.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fc0507f..d9cda7fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Frontend: update `dompurify` 3.4.2 → 3.4.11 (eight HTML-sanitizer XSS advisories — the markdown preview's runtime sanitizer), `vite` 8.0.0 → 8.0.16 (the `server.fs.deny` bypass and `__open-in-editor` NTLM dev-server CVEs), and add transitive overrides for `undici` → 7.28.0 (seven HTTP-client advisories via jsdom) and `@babel/core` → 7.29.6 (via vite-plugin-solid). The remaining flagged instance is the test-only vite 7.3.1 nested under vitest, whose fix is blocked by a JSX-transform incompatibility (documented as a justified, dev-server-only exception); the production bundle is built with the patched vite 8.0.16. This restores the weekly OSV scanner to green. ### Fixed +- Admin Command Center: the **7-day and 30-day** trend charts were always empty. Their daily-rollup materialized view could never refresh — its unique index was built on an expression, which PostgreSQL rejects for `REFRESH … CONCURRENTLY`, so the hourly refresh failed every cycle and the view stayed empty since it was introduced. The rollup now uses a plain-column unique index, and the refresh job falls back to a blocking refresh if a concurrent one ever can't proceed, so the long-range charts populate and stay current. (Shorter ranges — 1h/6h/24h — were unaffected as they read live data directly.) - Links in chat messages are now visible as links: they render in the theme's accent color with a soft underline (solid on hover), instead of looking identical to surrounding text. The styling is drawn from each theme's link color, so it adapts automatically across themes - Microphone test (voice settings): the level meter now fills from empty as you speak, instead of always showing a full green→red gradient. The overlay that reveals the bar was invisible because its background utility generated no CSS - Browser client: reloading the page no longer randomly logs you out. Two token-refresh requests raced at startup; the loser's 401 wiped the session the winner had just established (refresh requests are now single-flight) diff --git a/server/migrations/20260711000000_fix_command_center_rollup_index.sql b/server/migrations/20260711000000_fix_command_center_rollup_index.sql new file mode 100644 index 00000000..2074b98a --- /dev/null +++ b/server/migrations/20260711000000_fix_command_center_rollup_index.sql @@ -0,0 +1,38 @@ +-- Fix: the `telemetry_trend_rollups` materialized view could never be refreshed +-- with `REFRESH MATERIALIZED VIEW CONCURRENTLY`, so the hourly retention job +-- failed every cycle and the view stayed empty — the admin Command Center's +-- 7d/30d trend charts had no data. +-- +-- Cause: the unique index was built on an EXPRESSION (`COALESCE(route, '')`). +-- PostgreSQL only accepts a unique index on plain columns as the row-matching +-- index for a concurrent refresh; an expression index is ignored, which yields +-- `cannot refresh materialized view ... concurrently`. +-- +-- Fix: compute `route` as a non-null column inside the view (so NULL routes for +-- non-HTTP metrics collapse to '') and put the unique index on plain columns. + +DROP MATERIALIZED VIEW IF EXISTS telemetry_trend_rollups; + +CREATE MATERIALIZED VIEW telemetry_trend_rollups AS +SELECT + date_trunc('day', ts) AS day, + metric_name, + scope, + COALESCE(labels->>'http.route', '') AS route, + COUNT(*) AS sample_count, + AVG(value_p95) AS avg_p95, + MAX(value_p95) AS max_p95, + SUM(value_count) AS total_count, + SUM(CASE + WHEN labels->>'http.response.status_code' ~ '^\d+$' + AND (labels->>'http.response.status_code')::int >= 500 + THEN value_count + ELSE 0 + END) AS error_count +FROM telemetry_metric_samples +GROUP BY 1, 2, 3, 4; + +-- Plain-column unique index (no expression) so REFRESH ... CONCURRENTLY works. +-- `route` is now non-null, so it can participate directly. +CREATE UNIQUE INDEX idx_ttr_day_metric + ON telemetry_trend_rollups (day, metric_name, scope, route); diff --git a/server/src/observability/retention.rs b/server/src/observability/retention.rs index 85e3ad3c..f33c7654 100644 --- a/server/src/observability/retention.rs +++ b/server/src/observability/retention.rs @@ -148,9 +148,14 @@ async fn purge_in_batches(pool: &PgPool, sql: &'static str, table_label: &str) - total_deleted } -/// Refresh the trend rollups materialized view concurrently. +/// Refresh the trend rollups materialized view. /// -/// `CONCURRENTLY` allows reads during refresh (requires the unique index). +/// Prefers `CONCURRENTLY` (allows reads during refresh), which requires a +/// plain-column unique index and the view to have been populated at least once. +/// If that precondition isn't met the concurrent refresh errors out; rather than +/// leave the rollups silently stale we fall back to a blocking refresh, which has +/// no such requirement. The blocking refresh briefly locks the view, but that is +/// far better than the charts never updating. async fn refresh_trend_rollups(pool: &PgPool) { let start = Instant::now(); match sqlx::query("REFRESH MATERIALIZED VIEW CONCURRENTLY telemetry_trend_rollups") @@ -162,6 +167,25 @@ async fn refresh_trend_rollups(pool: &PgPool) { elapsed_ms = start.elapsed().as_millis() as u64, "Refreshed telemetry_trend_rollups" ); + return; + } + Err(e) => { + tracing::warn!( + error = %e, + "Concurrent rollup refresh failed; retrying without CONCURRENTLY" + ); + } + } + + match sqlx::query("REFRESH MATERIALIZED VIEW telemetry_trend_rollups") + .execute(pool) + .await + { + Ok(_) => { + tracing::debug!( + elapsed_ms = start.elapsed().as_millis() as u64, + "Refreshed telemetry_trend_rollups (non-concurrent fallback)" + ); } Err(e) => { tracing::warn!(error = %e, "Failed to refresh telemetry_trend_rollups"); From 5236f42e6629a6dad57ddb618fea2600bf9cdc1d Mon Sep 17 00:00:00 2001 From: Mal Detair Date: Sat, 11 Jul 2026 01:40:35 +0200 Subject: [PATCH 2/2] fix(observability): repair command-center summary/trends/top queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Command Center showed no data once telemetry accumulated — the endpoints errored rather than returning empty: - summary (500) and top-routes/top-errors: their SUM(value_count) over the bigint column returns NUMERIC, which failed to decode into i64. Cast the aggregates back to ::bigint. (Invisible on an empty DB: SUM over zero rows is NULL, which decodes fine — so it only broke in production with data.) - trends (400): the endpoint used axum's Query (serde_urlencoded), which can't parse repeated `metric=` params into a Vec. Switch to axum_extra's Query (serde_html_form); enable the axum-extra `query` feature. - 7d/30d rollups: the materialized view's SUM columns are also cast to ::bigint so the rollup trend query decodes. Adds an integration regression test that seeds samples and asserts the summary/top-routes/top-errors/trends queries decode with data present — the coverage gap that let this ship (previous tests ran against an empty DB). Verified against production data (types now decode as bigint; migration populates the matview). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JDEzVG39gGZFozPCndbSid --- CHANGELOG.md | 2 +- Cargo.lock | 17 ++++ Cargo.toml | 2 +- ...000000_fix_command_center_rollup_index.sql | 6 +- server/src/admin/observability.rs | 6 +- server/src/admin/queries.rs | 6 +- server/src/observability/storage.rs | 8 +- server/tests/integration/main.rs | 1 + .../integration/observability_queries.rs | 83 +++++++++++++++++++ 9 files changed, 120 insertions(+), 11 deletions(-) create mode 100644 server/tests/integration/observability_queries.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d9cda7fd..d61c69ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,7 +80,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Frontend: update `dompurify` 3.4.2 → 3.4.11 (eight HTML-sanitizer XSS advisories — the markdown preview's runtime sanitizer), `vite` 8.0.0 → 8.0.16 (the `server.fs.deny` bypass and `__open-in-editor` NTLM dev-server CVEs), and add transitive overrides for `undici` → 7.28.0 (seven HTTP-client advisories via jsdom) and `@babel/core` → 7.29.6 (via vite-plugin-solid). The remaining flagged instance is the test-only vite 7.3.1 nested under vitest, whose fix is blocked by a JSX-transform incompatibility (documented as a justified, dev-server-only exception); the production bundle is built with the patched vite 8.0.16. This restores the weekly OSV scanner to green. ### Fixed -- Admin Command Center: the **7-day and 30-day** trend charts were always empty. Their daily-rollup materialized view could never refresh — its unique index was built on an expression, which PostgreSQL rejects for `REFRESH … CONCURRENTLY`, so the hourly refresh failed every cycle and the view stayed empty since it was introduced. The rollup now uses a plain-column unique index, and the refresh job falls back to a blocking refresh if a concurrent one ever can't proceed, so the long-range charts populate and stay current. (Shorter ranges — 1h/6h/24h — were unaffected as they read live data directly.) +- Admin Command Center showed no data (empty vital signs and charts) once telemetry had accumulated. Several fixes: (1) the **summary** endpoint returned 500 and the **top-routes/top-errors** panels failed because their `SUM()` aggregates over a `bigint` column come back as `NUMERIC` and could not decode into an integer — they now cast back to `bigint` (this was invisible on an empty database, where `SUM` is `NULL`); (2) the **trend charts** returned 400 because the endpoint could not parse its repeated `metric=` query parameters into a list — it now uses a multi-value query parser; (3) the **7-day and 30-day** rollups were additionally empty because their materialized view could never `REFRESH … CONCURRENTLY` (its unique index was on an expression, which PostgreSQL rejects) — the view now uses a plain-column unique index and the refresh job falls back to a blocking refresh if a concurrent one can't proceed. - Links in chat messages are now visible as links: they render in the theme's accent color with a soft underline (solid on hover), instead of looking identical to surrounding text. The styling is drawn from each theme's link color, so it adapts automatically across themes - Microphone test (voice settings): the level meter now fills from empty as you speak, instead of always showing a full green→red gradient. The overlay that reveals the bar was invisible because its background utility generated no CSS - Browser client: reloading the page no longer randomly logs you out. Two token-refresh requests raced at startup; the loser's 401 wiped the session the winner had just established (refresh requests are now single-flight) diff --git a/Cargo.lock b/Cargo.lock index e6b05006..6c9ca468 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1178,6 +1178,7 @@ dependencies = [ "axum-core", "bytes", "cookie", + "form_urlencoded", "futures-core", "futures-util", "http 1.4.2", @@ -1185,6 +1186,9 @@ dependencies = [ "http-body-util", "mime", "pin-project-lite", + "serde_core", + "serde_html_form", + "serde_path_to_error", "tower-layer", "tower-service", "tracing", @@ -8923,6 +8927,19 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "serde_html_form" +version = "0.2.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b2f2d7ff8a2140333718bb329f5c40fc5f0865b84c426183ce14c97d2ab8154f" +dependencies = [ + "form_urlencoded", + "indexmap 2.14.0", + "itoa", + "ryu", + "serde_core", +] + [[package]] name = "serde_json" version = "1.0.150" diff --git a/Cargo.toml b/Cargo.toml index 806ad602..485a3ef6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,7 @@ futures = "0.3" # Web Framework axum = { version = "0.8", features = ["ws", "multipart", "macros"] } -axum-extra = { version = "0.12", features = ["cookie"] } +axum-extra = { version = "0.12", features = ["cookie", "query"] } time = "0.3" tower = "0.5" tower-http = { version = "0.6", features = ["cors", "trace", "compression-gzip", "request-id", "util"] } diff --git a/server/migrations/20260711000000_fix_command_center_rollup_index.sql b/server/migrations/20260711000000_fix_command_center_rollup_index.sql index 2074b98a..a37c0896 100644 --- a/server/migrations/20260711000000_fix_command_center_rollup_index.sql +++ b/server/migrations/20260711000000_fix_command_center_rollup_index.sql @@ -22,13 +22,15 @@ SELECT COUNT(*) AS sample_count, AVG(value_p95) AS avg_p95, MAX(value_p95) AS max_p95, - SUM(value_count) AS total_count, + -- SUM() over a bigint column yields NUMERIC; cast back to bigint so the + -- trend query decodes total_count/error_count into i64. + SUM(value_count)::bigint AS total_count, SUM(CASE WHEN labels->>'http.response.status_code' ~ '^\d+$' AND (labels->>'http.response.status_code')::int >= 500 THEN value_count ELSE 0 - END) AS error_count + END)::bigint AS error_count FROM telemetry_metric_samples GROUP BY 1, 2, 3, 4; diff --git a/server/src/admin/observability.rs b/server/src/admin/observability.rs index 65037206..5cf215df 100644 --- a/server/src/admin/observability.rs +++ b/server/src/admin/observability.rs @@ -7,8 +7,12 @@ use std::time::Instant; -use axum::extract::{Query, State}; +use axum::extract::State; use axum::{Extension, Json}; +// `axum_extra`'s Query (serde_html_form) supports repeated keys like +// `?metric=a&metric=b` into a `Vec`; axum's built-in Query (serde_urlencoded) +// does not and would 400 on the trends endpoint's multiple `metric` params. +use axum_extra::extract::Query; use chrono::{DateTime, Duration, Utc}; use futures::future::try_join_all; use serde::{Deserialize, Serialize}; diff --git a/server/src/admin/queries.rs b/server/src/admin/queries.rs index d685d2dd..8ca61a86 100644 --- a/server/src/admin/queries.rs +++ b/server/src/admin/queries.rs @@ -949,10 +949,12 @@ pub async fn summary_error_and_request_counts( from: DateTime, to: DateTime, ) -> Result, Option)>, sqlx::Error> { + // SUM() over a bigint column returns NUMERIC in PostgreSQL; cast back to + // bigint so it decodes into i64 (the sums are small request counts). sqlx::query_as::<_, (Option, Option)>( "SELECT \ - SUM(CASE WHEN metric_name = 'kaiku_http_errors_total' THEN value_count ELSE 0 END), \ - SUM(CASE WHEN metric_name = 'kaiku_http_requests_total' THEN value_count ELSE 0 END) \ + SUM(CASE WHEN metric_name = 'kaiku_http_errors_total' THEN value_count ELSE 0 END)::bigint, \ + SUM(CASE WHEN metric_name = 'kaiku_http_requests_total' THEN value_count ELSE 0 END)::bigint \ FROM telemetry_metric_samples \ WHERE metric_name IN ('kaiku_http_errors_total', 'kaiku_http_requests_total') \ AND ts >= $1 AND ts <= $2", diff --git a/server/src/observability/storage.rs b/server/src/observability/storage.rs index 2a0b256f..5d9ff830 100644 --- a/server/src/observability/storage.rs +++ b/server/src/observability/storage.rs @@ -411,11 +411,11 @@ pub async fn query_top_routes( const BASE: &str = "\ SELECT \ labels->>'http.route' AS route, \ - SUM(value_count) AS request_count, \ + SUM(value_count)::bigint AS request_count, \ SUM(CASE \ WHEN labels->>'http.response.status_code' ~ '^\\d+$' \ AND (labels->>'http.response.status_code')::int >= 500 \ - THEN value_count ELSE 0 END) AS error_count, \ + THEN value_count ELSE 0 END)::bigint AS error_count, \ AVG(value_p95) AS avg_p95, \ MAX(value_p95) AS max_p95 \ FROM telemetry_metric_samples \ @@ -457,8 +457,8 @@ pub async fn query_top_errors( sqlx::query_as::<_, TopRouteEntry>( "SELECT \ labels->>'error.type' AS route, \ - SUM(value_count) AS request_count, \ - SUM(value_count) AS error_count, \ + SUM(value_count)::bigint AS request_count, \ + SUM(value_count)::bigint AS error_count, \ AVG(value_p95) AS avg_p95, \ MAX(value_p95) AS max_p95 \ FROM telemetry_metric_samples \ diff --git a/server/tests/integration/main.rs b/server/tests/integration/main.rs index 0ba27461..b3c28a52 100644 --- a/server/tests/integration/main.rs +++ b/server/tests/integration/main.rs @@ -29,6 +29,7 @@ mod identities_http; mod media_processing; mod mention_permission; mod messages_http; +mod observability_queries; mod oidc; mod pages; mod performance_budgets; diff --git a/server/tests/integration/observability_queries.rs b/server/tests/integration/observability_queries.rs new file mode 100644 index 00000000..4b231091 --- /dev/null +++ b/server/tests/integration/observability_queries.rs @@ -0,0 +1,83 @@ +//! Regression tests for the admin Command Center observability queries. +//! +//! Several of these queries `SUM(value_count)` over a `bigint` column, which +//! `PostgreSQL` returns as `NUMERIC`. Without a `::bigint` cast the row fails to +//! decode into `i64` — but only once there is data, because `SUM` over zero +//! rows is `NULL`, which decodes fine. The bug was therefore invisible in an +//! empty test database and only surfaced in production once telemetry +//! accumulated (summary → 500, top-routes/top-errors → 500). These tests seed +//! samples first, then assert the queries succeed and decode. + +use chrono::{Duration, Utc}; +use serde_json::json; +use sqlx::PgPool; +use vc_server::observability::storage::{self, InsertMetricSample}; + +async fn insert_sample( + pool: &PgPool, + metric: &str, + labels: &serde_json::Value, + count: i64, + p95: f64, +) { + storage::insert_metric_sample( + pool, + &InsertMetricSample { + ts: Utc::now(), + metric_name: metric, + scope: "test", + labels, + value_count: Some(count), + value_sum: None, + value_p50: None, + value_p95: Some(p95), + value_p99: None, + }, + ) + .await + .expect("insert metric sample"); +} + +#[sqlx::test] +async fn observability_aggregate_queries_decode_with_data(pool: PgPool) { + let route = json!({ "http.route": "/api/x", "http.response.status_code": "200" }); + let route_err = json!({ "http.route": "/api/x", "http.response.status_code": "500" }); + let err_typed = json!({ "error.type": "timeout" }); + + insert_sample(&pool, "kaiku_http_request_duration_ms", &route, 10, 12.5).await; + insert_sample(&pool, "kaiku_http_request_duration_ms", &route_err, 3, 40.0).await; + insert_sample(&pool, "kaiku_http_requests_total", &route, 100, 0.0).await; + insert_sample(&pool, "kaiku_http_errors_total", &err_typed, 5, 0.0).await; + + let from = Utc::now() - Duration::hours(1); + let to = Utc::now() + Duration::minutes(1); + + // top_routes: SUM(value_count)::bigint and the 500-status error SUM must decode. + let routes = storage::query_top_routes(&pool, from, to, false, 10) + .await + .expect("query_top_routes must decode with data present"); + assert_eq!(routes.len(), 1, "expected one route row"); + assert_eq!(routes[0].request_count, Some(13)); // 10 + 3 + assert_eq!(routes[0].error_count, Some(3)); // the 500 sample + + // top_errors: SUM(value_count)::bigint grouped by error.type must decode. + let errors = storage::query_top_errors(&pool, from, to, 10) + .await + .expect("query_top_errors must decode with data present"); + assert_eq!(errors.len(), 1, "expected one error-type row"); + assert_eq!(errors[0].error_count, Some(5)); + + // trends (<=24h reads raw samples directly). + let trend = storage::query_trends(&pool, "kaiku_http_requests_total", from, to) + .await + .expect("query_trends must decode"); + assert!(!trend.is_empty(), "expected trend datapoints"); + + // summary error/request counts: SUM(...)::bigint must decode. + let counts = vc_server::admin::queries::summary_error_and_request_counts(&pool, from, to) + .await + .expect("summary_error_and_request_counts must decode") + .expect("counts row present"); + assert_eq!(counts.0, Some(5), "error count"); + assert_eq!(counts.1, Some(100), "request count"); +}