diff --git a/packages/prices-clickhouse/src/lib.rs b/packages/prices-clickhouse/src/lib.rs index 7972322a..54d7e271 100644 --- a/packages/prices-clickhouse/src/lib.rs +++ b/packages/prices-clickhouse/src/lib.rs @@ -6,7 +6,7 @@ //! crates own their own row structs and writers; this crate only stands up the //! schema and hands out a configured client. -use clickhouse::Client; +use clickhouse::{Client, Compression}; /// Optional env-var helpers (`env_or` / `env_parse_or`) shared by the worker /// Lambdas. Companion to `mtls::require_env` (the must-be-set case). @@ -199,6 +199,34 @@ pub fn with_execution_bound(client: Client, secs: u64) -> Client { } } +/// Configure `client` so a failed statement reaches the caller carrying +/// ClickHouse's own message (task 0281). +/// +/// ⚠️ **Not a performance setting.** With the crate's default +/// `Compression::Lz4`, EVERY ClickHouse error arrives as `BadResponse("")` — +/// an empty string. Not only timeouts: unknown table, syntax error, quota +/// exceeded and disk-full all lose their code and message, becoming +/// indistinguishable from a network blip. That is the signature that hid task +/// 0215's outage for 26 days. +/// +/// The mechanism is a fallback that fails to fire. `collect_bad_response` +/// LZ4-decodes the error body and falls back to the raw bytes on failure: +/// +/// ```text +/// let bytes = collect_bytes(stream).await.unwrap_or(raw_bytes); +/// ``` +/// +/// Straight to ClickHouse the decode fails, the fallback fires, the message +/// survives. Through a proxy the chunk reframing makes the decode *succeed* +/// with zero bytes, so `unwrap_or` never runs. Every production client reaches +/// ClickHouse through Caddy. +/// +/// Every client built in this crate must go through here, so the guard cannot +/// be lost by someone constructing a client a different way. +pub fn with_readable_errors(client: Client) -> Client { + client.with_compression(Compression::None) +} + /// The effective per-statement bound for a configured value: `None` when there /// is none. /// diff --git a/packages/prices-clickhouse/src/mtls.rs b/packages/prices-clickhouse/src/mtls.rs index 18572990..630bc21b 100644 --- a/packages/prices-clickhouse/src/mtls.rs +++ b/packages/prices-clickhouse/src/mtls.rs @@ -305,9 +305,12 @@ pub fn client_with_mtls( .build(https); let url = format!("https://{domain}"); - Ok(clickhouse::Client::with_http_client(hyper_client) - .with_url(url) - .with_database(database)) + // ⚠️ `with_readable_errors` is load-bearing, not cosmetic: without it every + // ClickHouse error arrives as an empty string. See its docs (task 0281). + Ok(crate::with_readable_errors( + clickhouse::Client::with_http_client(hyper_client).with_url(url), + ) + .with_database(database)) } /// Build an mTLS ClickHouse client from PEM **file paths** (client cert, client diff --git a/packages/prices-clickhouse/tests/execution_bound_error_it.rs b/packages/prices-clickhouse/tests/execution_bound_error_it.rs new file mode 100644 index 00000000..6a4b9335 --- /dev/null +++ b/packages/prices-clickhouse/tests/execution_bound_error_it.rs @@ -0,0 +1,109 @@ +//! A statement killed by `max_execution_time` must reach the caller carrying +//! ClickHouse's own error — the code, the elapsed time and the bound crossed +//! (task 0281). +//! +//! docker compose up -d clickhouse +//! cargo test -p prices-clickhouse --test execution_bound_error_it -- --ignored +//! +//! WHY THIS EXISTS +//! --------------- +//! On 2026-09-11 the enrichment worker's bound was induced on production. The +//! bound fired exactly — ClickHouse killed the statement at 1002.3 ms against +//! a 1000 ms limit and recorded +//! +//! Code: 159. DB::Exception: Timeout exceeded: elapsed 1002.319598 ms, +//! maximum: 1000 ms. (TIMEOUT_EXCEEDED) +//! +//! while the worker logged `clickhouse: bad response: ` — an EMPTY string. +//! Every fact needed to diagnose the failure existed on the wire and was lost +//! one layer above it. That empty error is indistinguishable from a network +//! blip, which is exactly the signature that hid task 0215's outage for 26 +//! days. +//! +//! ⚠️ The statement must be an `INSERT … SELECT`, not a `SELECT`. The two take +//! different paths, and the difference is what made this invisible: on +//! 2026-09-10 twenty-three `TIMEOUT_EXCEEDED` events were observed arriving +//! complete and well-formed — every one of them a read. + +use clickhouse::Client; + +fn ch_url() -> String { + std::env::var("CLICKHOUSE_URL").unwrap_or_else(|_| "http://localhost:8123".to_string()) +} + +/// The URL of a **reverse proxy** in front of ClickHouse. +/// +/// ⛔ This test is worthless without one, and would pass anyway — which is why +/// its absence fails rather than skips. The defect only appears through a +/// proxy: straight to ClickHouse the crate's LZ4 fallback fires and the error +/// survives even when the bug is present. Production always has Caddy in the +/// path, so the proxy IS the production shape. +/// +/// scripts/ch-proxy-0281.sh up # Caddy on :8124 -> ClickHouse :8123 +fn proxy_url() -> String { + std::env::var("CLICKHOUSE_PROXY_URL").expect( + "CLICKHOUSE_PROXY_URL is unset — this test cannot detect the defect without a \ + reverse proxy in front of ClickHouse, and would pass vacuously. Run \ + `scripts/ch-proxy-0281.sh up` and re-run with \ + CLICKHOUSE_PROXY_URL=http://localhost:8124", + ) +} + +/// An `INSERT … SELECT` guaranteed to outrun a one-second bound: it generates +/// far more rows than any machine can insert in that time, from `numbers` so +/// the cost is CPU rather than disk. +const SLOW_INSERT: &str = "INSERT INTO it_0281.sink SELECT number FROM numbers(5000000000)"; + +async fn setup() -> Client { + let client = Client::default().with_url(ch_url()); + client + .query("DROP DATABASE IF EXISTS it_0281") + .execute() + .await + .unwrap(); + client + .query("CREATE DATABASE it_0281") + .execute() + .await + .unwrap(); + client + .query("CREATE TABLE it_0281.sink (n UInt64) ENGINE = MergeTree ORDER BY n") + .execute() + .await + .unwrap(); + client +} + +#[tokio::test] +#[ignore = "requires a local ClickHouse (docker compose up -d clickhouse)"] +async fn an_exceeded_bound_reaches_the_caller_as_a_clickhouse_exception() { + setup().await; + + // Built exactly as production builds it: through `with_readable_errors`, + // the same function `mtls::client_with_mtls` uses. A change there fails + // this test. + let client = prices_clickhouse::with_readable_errors(Client::default().with_url(proxy_url())); + let bounded = prices_clickhouse::with_execution_bound(client, 1); + + let err = bounded + .query(SLOW_INSERT) + .execute() + .await + .expect_err("a 5-billion-row INSERT cannot finish inside a 1s bound"); + + let text = err.to_string(); + + // The three facts that make a failure diagnosable after the fact. Asserted + // separately so a regression says WHICH one was lost. + assert!( + !text + .trim_end_matches(|c: char| c == ':' || c.is_whitespace()) + .ends_with("bad response"), + "the error carries no message at all — this is the 0281 defect: {text:?}" + ); + assert!(text.contains("159"), "the error code is missing: {text:?}"); + assert!( + text.contains("TIMEOUT_EXCEEDED"), + "the error name is missing: {text:?}" + ); +} diff --git a/scripts/ch-proxy-0281.sh b/scripts/ch-proxy-0281.sh new file mode 100755 index 00000000..6d9296ae --- /dev/null +++ b/scripts/ch-proxy-0281.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# +# Stand a reverse proxy in front of the local ClickHouse, so tests can exercise +# the PRODUCTION shape (task 0281). +# +# WHY THIS EXISTS +# --------------- +# `execution_bound_error_it` asserts that a failed statement reaches the caller +# carrying ClickHouse's own message. Straight to ClickHouse it always does — +# even with the defect present — because the crate's LZ4 fallback fires. Put a +# proxy in the path and the decode succeeds with zero bytes instead, the +# fallback never runs, and the message becomes "". Production always has Caddy +# in front of ClickHouse, so without a proxy the test is vacuous. +# +# scripts/ch-proxy-0281.sh up +# CLICKHOUSE_PROXY_URL=http://localhost:8124 \ +# cargo test -p prices-clickhouse --test execution_bound_error_it -- --ignored +# scripts/ch-proxy-0281.sh down +set -euo pipefail + +name=ch-proxy-0281 +conf="$(mktemp -d)/Caddyfile" + +case "${1:-up}" in + up) + # Mirrors the transport block BE runs in front of ch-prod-01. + cat > "$conf" <<'EOF' +{ + admin off + auto_https off +} + +:8124 { + reverse_proxy localhost:8123 { + transport http { + dial_timeout 10s + response_header_timeout 7200s + read_timeout 7200s + write_timeout 7200s + } + } +} +EOF + docker rm -f "$name" >/dev/null 2>&1 || true + docker run -d --name "$name" --network host \ + -v "$conf":/etc/caddy/Caddyfile:ro caddy:2 >/dev/null + until curl -sS --max-time 2 "http://localhost:8124/?query=SELECT%201" >/dev/null 2>&1; do + sleep 1 + done + echo "proxy up: http://localhost:8124 -> clickhouse :8123" + ;; + down) + docker rm -f "$name" >/dev/null 2>&1 || true + echo "proxy down" + ;; + *) + echo "usage: $0 [up|down]" >&2 + exit 1 + ;; +esac