From 535073011b08e7367a604b1e7d49817bf3007ffe Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 29 Sep 2026 01:53:53 +0200 Subject: [PATCH] hive-forge: bound the web-router client's connect and request waits The raw reqwest client hive-forge uses for Forgejo's web-router-only routes (attachment downloads, Actions artifact/log routes) had no timeout, so a hung Forgejo response blocked the calling CLI invocation indefinitely. Adds a 5s connect_timeout (matching #4737's outbound-HTTP sites) plus a per-call request timeout: 15s (config_pr_poll's forge budget) for the small JSON calls (get_api_json, post_json_web), and 10 minutes for get_bytes_named/get_bytes_raw, which download attachments, Actions artifact zips and persisted job logs that can be large. reqwest::blocking has no separate read/idle timeout, so a single whole-request budget has to cover those downloads; the smaller JSON budget would cut them off partway through. Refs #4746, #4737 --- hive-forge/src/client.rs | 70 ++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/hive-forge/src/client.rs b/hive-forge/src/client.rs index cfe86bdb..8df1042f 100644 --- a/hive-forge/src/client.rs +++ b/hive-forge/src/client.rs @@ -20,6 +20,19 @@ use serde_json::Value; /// Default Forgejo URL when `HIVE_FORGE_URL` is unset. const DEFAULT_URL: &str = "http://localhost:3000"; +/// Bound on reaching the forge. +const HTTP_CONNECT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); +/// Whole-request budget for a small JSON call (an `/api/v1` GET, or the +/// run-view log streamer's snapshot POST) — `config_pr_poll`'s forge budget. +const JSON_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(15); +/// Whole-request budget for a GET that downloads a file body — an +/// attachment, an Actions artifact zip, or a persisted job log. Sized +/// for a large-but-live transfer rather than a JSON call: +/// `reqwest::blocking` has no separate read/idle timeout, so this bounds +/// the whole download rather than cutting it off partway through like +/// [`JSON_TIMEOUT`] would. +const DOWNLOAD_TIMEOUT: std::time::Duration = std::time::Duration::from_mins(10); + /// Forgejo client pair: the typed `/api/v1` client plus a raw /// `reqwest` client for web-router-only routes. pub struct Client { @@ -95,6 +108,7 @@ impl Client { headers.insert(ACCEPT, HeaderValue::from_static("application/json")); let web = HttpClient::builder() .default_headers(headers) + .connect_timeout(HTTP_CONNECT_TIMEOUT) .build() .context("build reqwest client")?; @@ -206,11 +220,17 @@ impl Client { .post(&url) .header(CONTENT_TYPE, "application/json") .json(body) + .timeout(JSON_TIMEOUT) .send() - .context("POST")?; + .map_err(|e| { + let ctx = http_context("POST", &e, JSON_TIMEOUT); + anyhow::Error::new(e).context(ctx) + })?; let resp = check_status(resp, &format!("POST {url}"))?; - resp.json::() - .with_context(|| format!("decode JSON for POST {url}")) + resp.json::().map_err(|e| { + let ctx = http_context(&format!("decode JSON for POST {url}"), &e, JSON_TIMEOUT); + anyhow::Error::new(e).context(ctx) + }) } /// GET a raw (non-API) URL and return the response body as bytes. @@ -236,16 +256,25 @@ impl Client { /// # Errors /// Same as [`Client::get_bytes_raw`] — transport failure or a non-2xx. pub fn get_bytes_named(&self, url: &str) -> Result<(Vec, Option)> { - let resp = self.web.get(url).send().context("GET")?; + let resp = self + .web + .get(url) + .timeout(DOWNLOAD_TIMEOUT) + .send() + .map_err(|e| { + let ctx = http_context("GET", &e, DOWNLOAD_TIMEOUT); + anyhow::Error::new(e).context(ctx) + })?; let resp = check_status(resp, &format!("GET {url}"))?; let name = resp .headers() .get(CONTENT_DISPOSITION) .and_then(|v| v.to_str().ok()) .and_then(disposition_filename); - resp.bytes() - .map(|b| (b.to_vec(), name)) - .with_context(|| format!("read bytes for GET {url}")) + resp.bytes().map(|b| (b.to_vec(), name)).map_err(|e| { + let ctx = http_context(&format!("read bytes for GET {url}"), &e, DOWNLOAD_TIMEOUT); + anyhow::Error::new(e).context(ctx) + }) } /// GET a JSON `/api/v1` route and deserialize into `T`, bypassing the @@ -269,10 +298,20 @@ impl Client { let base = format!("{}/api/v1{path}", self.base); let url = url::Url::parse_with_params(&base, query.iter().copied()) .with_context(|| format!("build url {base}"))?; - let resp = self.web.get(url.clone()).send().context("GET")?; + let resp = self + .web + .get(url.clone()) + .timeout(JSON_TIMEOUT) + .send() + .map_err(|e| { + let ctx = http_context("GET", &e, JSON_TIMEOUT); + anyhow::Error::new(e).context(ctx) + })?; let resp = check_status(resp, &format!("GET {url}"))?; - resp.json::() - .with_context(|| format!("decode JSON for GET {url}")) + resp.json::().map_err(|e| { + let ctx = http_context(&format!("decode JSON for GET {url}"), &e, JSON_TIMEOUT); + anyhow::Error::new(e).context(ctx) + }) } } @@ -526,6 +565,17 @@ impl std::fmt::Display for WebStatusError { impl std::error::Error for WebStatusError {} +/// `op` failed with `e`; a timeout names the bound that fired. +fn http_context(op: &str, e: &reqwest::Error, budget: std::time::Duration) -> String { + if e.is_connect() && e.is_timeout() { + format!("{op}: connect timed out after {HTTP_CONNECT_TIMEOUT:?}") + } else if e.is_timeout() { + format!("{op}: timed out after {budget:?}") + } else { + op.to_owned() + } +} + /// Surface non-2xx HTTP responses on the raw web routes as a /// [`WebStatusError`] with the response body included (matches `curl /// --fail-with-body`) — turns silent failures into errors with a