From 7d9c5c9eb93c58ce12fb717920ee8da969d0518b Mon Sep 17 00:00:00 2001 From: Jeff Larson Date: Sat, 25 Jul 2026 16:00:04 -0700 Subject: [PATCH] fix(server): bound alert webhook/SMTP delivery timeouts (JEF-497) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit alert.notify spans clustered at ~11.5s (11,494/11,604/11,750ms, error_count=0) — slow, not failing — a fixed connect/handshake stall in the delivery path. Both notify() sinks were unbounded: - Webhook used reqwest::Client::new(), which has no request AND no connect timeout, so a slow/unresponsive receiver blocked send() up to the OS TCP timeout. - The lettre AsyncSmtpTransport was built without .timeout(...), so a slow SMTP relay handshake stalled just as long. Because notify() is awaited inside the eval tick (run → evaluate → notify), a stalled sink delayed the whole tick and backed up other rules' notifications. Fix: build the webhook client once with explicit bounds (10s request / 5s connect) via a shared build_webhook_client() helper so run and evaluate_once get identical config; a failed build logs and disables the webhook rather than panicking, matching how a bad SMTP config disables email. Give the SMTP transport .timeout(Some(10s)) in Mailer::build. Values are short enough a dead sink can't stall the loop, long enough for a legitimately slow-but-alive receiver. Test: a local TcpListener that accepts but never responds (the exact hung- receiver case) makes notify() give up in ~10s (measured 10.01s), asserted < 15s — bounded by the request timeout, not the multi-second OS TCP timeout. Re-architecting the loop (spawning notify off the tick / a delivery queue) is a noted follow-up and out of scope here. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01JbrmfzHsTMMzPaSrUkZgWo --- server/src/alerts.rs | 81 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 77 insertions(+), 4 deletions(-) diff --git a/server/src/alerts.rs b/server/src/alerts.rs index 9e7802f..7c4fc5d 100644 --- a/server/src/alerts.rs +++ b/server/src/alerts.rs @@ -236,6 +236,9 @@ impl Mailer { let transport = AsyncSmtpTransport::::starttls_relay(&cfg.relay)? .port(cfg.port) .credentials(Credentials::new(cfg.username.clone(), cfg.password.clone())) + // Bound the SMTP handshake/send: a slow relay must give up in seconds so + // it can't stall the alert-eval loop (`notify` is awaited on the tick). + .timeout(Some(Duration::from_secs(10))) .build(); Ok(Self { transport, @@ -369,6 +372,19 @@ fn breached(value: Option, comparator: &str, threshold: f64) -> bool { } } +/// The HTTP client for webhook delivery, bounded so a slow/unresponsive receiver +/// can't stall the eval loop (`notify` is awaited on the tick): 10s total per +/// request, 5s to connect — short enough a dead sink gives up in seconds, long +/// enough for a legitimately slow-but-alive receiver. Shared by `run` and +/// `evaluate_once` so both delivery paths get identical bounds. Fallible (the TLS +/// backend init can fail); callers log and disable the webhook rather than panic. +fn build_webhook_client() -> reqwest::Result { + reqwest::Client::builder() + .timeout(Duration::from_secs(10)) + .connect_timeout(Duration::from_secs(5)) + .build() +} + /// Runs forever; ticks immediately, then every `interval_secs`. pub async fn run( pool: PgPool, @@ -376,7 +392,15 @@ pub async fn run( email: Option, interval_secs: u64, ) { - let client = reqwest::Client::new(); + // A failed client build disables the webhook (logged) rather than taking down + // the alert loop, consistent with how a bad SMTP config disables email below. + let (client, webhook) = match build_webhook_client() { + Ok(c) => (c, webhook), + Err(e) => { + tracing::error!("alert webhook disabled — failed to build HTTP client: {e}"); + (reqwest::Client::new(), None) + } + }; // Build the mailer once. A bad address/relay disables email (logged) rather // than taking down the alert loop. let mailer = match email.as_ref().map(Mailer::build) { @@ -400,7 +424,14 @@ pub async fn run( /// Exposed for tests and one-shot use; `run` calls it on a timer. Email is only /// wired through `run` (it owns the built mailer), so this path is webhook-only. pub async fn evaluate_once(pool: &PgPool, webhook: Option<&str>) -> anyhow::Result<()> { - let client = reqwest::Client::new(); + // Same bounded client as `run`; a failed build disables the webhook (logged). + let (client, webhook) = match build_webhook_client() { + Ok(c) => (c, webhook), + Err(e) => { + tracing::error!("alert webhook disabled — failed to build HTTP client: {e}"); + (reqwest::Client::new(), None) + } + }; evaluate(pool, webhook, None, &client).await } @@ -598,8 +629,8 @@ async fn notify( #[cfg(test)] mod tests { use super::{ - agg_expr, breached, email_body, email_subject, eval_sql, validate, AlertRule, RuleConfig, - MAX_FOR_SECS, + agg_expr, breached, build_webhook_client, email_body, email_subject, eval_sql, notify, + validate, AlertRule, RuleConfig, MAX_FOR_SECS, }; #[test] @@ -813,6 +844,48 @@ mod tests { assert!(!sql.contains("lag(value)")); } + // A webhook receiver that accepts the TCP connection but never sends a byte of + // response — the exact "slow/unresponsive receiver" that used to stall `notify` + // for ~11.5s. The bounded client's 10s request timeout must make `notify` give + // up well within the margin rather than blocking on the OS TCP timeout. + #[tokio::test] + async fn notify_gives_up_on_a_hung_webhook_within_timeout() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + // Accept forever, holding each connection open without ever responding. + tokio::spawn(async move { + let mut held = Vec::new(); + while let Ok((sock, _)) = listener.accept().await { + held.push(sock); // keep it alive; never write back + } + }); + + let client = build_webhook_client().unwrap(); + let url = format!("http://{addr}/hook"); + let rule = rule(); + + let start = std::time::Instant::now(); + // No mailer — this exercises the webhook sink in isolation. + notify( + &client, + Some(url.as_str()), + None, + &rule, + "firing", + Some(87.5), + ) + .await; + let elapsed = start.elapsed(); + + // The call must complete (it returns unit even on error, logging the failure) + // bounded by the 10s request timeout — never the multi-second-to-minutes OS + // TCP timeout. Generous 15s ceiling keeps it non-flaky under load. + assert!( + elapsed < std::time::Duration::from_secs(15), + "notify() should give up on a hung webhook within the bounded timeout, took {elapsed:?}" + ); + } + #[test] fn eval_sql_rate_differences_reset_safe() { let sql = eval_sql("max", true);