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
This commit is contained in:
parent
202f7f7c83
commit
535073011b
1 changed files with 60 additions and 10 deletions
|
|
@ -20,6 +20,19 @@ use serde_json::Value;
|
||||||
/// Default Forgejo URL when `HIVE_FORGE_URL` is unset.
|
/// Default Forgejo URL when `HIVE_FORGE_URL` is unset.
|
||||||
const DEFAULT_URL: &str = "http://localhost:3000";
|
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
|
/// Forgejo client pair: the typed `/api/v1` client plus a raw
|
||||||
/// `reqwest` client for web-router-only routes.
|
/// `reqwest` client for web-router-only routes.
|
||||||
pub struct Client {
|
pub struct Client {
|
||||||
|
|
@ -95,6 +108,7 @@ impl Client {
|
||||||
headers.insert(ACCEPT, HeaderValue::from_static("application/json"));
|
headers.insert(ACCEPT, HeaderValue::from_static("application/json"));
|
||||||
let web = HttpClient::builder()
|
let web = HttpClient::builder()
|
||||||
.default_headers(headers)
|
.default_headers(headers)
|
||||||
|
.connect_timeout(HTTP_CONNECT_TIMEOUT)
|
||||||
.build()
|
.build()
|
||||||
.context("build reqwest client")?;
|
.context("build reqwest client")?;
|
||||||
|
|
||||||
|
|
@ -206,11 +220,17 @@ impl Client {
|
||||||
.post(&url)
|
.post(&url)
|
||||||
.header(CONTENT_TYPE, "application/json")
|
.header(CONTENT_TYPE, "application/json")
|
||||||
.json(body)
|
.json(body)
|
||||||
|
.timeout(JSON_TIMEOUT)
|
||||||
.send()
|
.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}"))?;
|
let resp = check_status(resp, &format!("POST {url}"))?;
|
||||||
resp.json::<Value>()
|
resp.json::<Value>().map_err(|e| {
|
||||||
.with_context(|| format!("decode JSON for POST {url}"))
|
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.
|
/// GET a raw (non-API) URL and return the response body as bytes.
|
||||||
|
|
@ -236,16 +256,25 @@ impl Client {
|
||||||
/// # Errors
|
/// # Errors
|
||||||
/// Same as [`Client::get_bytes_raw`] — transport failure or a non-2xx.
|
/// Same as [`Client::get_bytes_raw`] — transport failure or a non-2xx.
|
||||||
pub fn get_bytes_named(&self, url: &str) -> Result<(Vec<u8>, Option<String>)> {
|
pub fn get_bytes_named(&self, url: &str) -> Result<(Vec<u8>, Option<String>)> {
|
||||||
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 resp = check_status(resp, &format!("GET {url}"))?;
|
||||||
let name = resp
|
let name = resp
|
||||||
.headers()
|
.headers()
|
||||||
.get(CONTENT_DISPOSITION)
|
.get(CONTENT_DISPOSITION)
|
||||||
.and_then(|v| v.to_str().ok())
|
.and_then(|v| v.to_str().ok())
|
||||||
.and_then(disposition_filename);
|
.and_then(disposition_filename);
|
||||||
resp.bytes()
|
resp.bytes().map(|b| (b.to_vec(), name)).map_err(|e| {
|
||||||
.map(|b| (b.to_vec(), name))
|
let ctx = http_context(&format!("read bytes for GET {url}"), &e, DOWNLOAD_TIMEOUT);
|
||||||
.with_context(|| format!("read bytes for GET {url}"))
|
anyhow::Error::new(e).context(ctx)
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
/// GET a JSON `/api/v1` route and deserialize into `T`, bypassing the
|
/// 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 base = format!("{}/api/v1{path}", self.base);
|
||||||
let url = url::Url::parse_with_params(&base, query.iter().copied())
|
let url = url::Url::parse_with_params(&base, query.iter().copied())
|
||||||
.with_context(|| format!("build url {base}"))?;
|
.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}"))?;
|
let resp = check_status(resp, &format!("GET {url}"))?;
|
||||||
resp.json::<T>()
|
resp.json::<T>().map_err(|e| {
|
||||||
.with_context(|| format!("decode JSON for GET {url}"))
|
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 {}
|
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
|
/// Surface non-2xx HTTP responses on the raw web routes as a
|
||||||
/// [`WebStatusError`] with the response body included (matches `curl
|
/// [`WebStatusError`] with the response body included (matches `curl
|
||||||
/// --fail-with-body`) — turns silent failures into errors with a
|
/// --fail-with-body`) — turns silent failures into errors with a
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue