fix(maestro): give CLI reqwest clients connect + request timeouts (NAP-262)
ci / rust (push) Successful in 9m49s
ci / rust (push) Successful in 9m49s
The `RegistryClient` and the bare `reqwest::Client::new()` calls in
`policy.rs` built clients with no timeouts, so `maestro list` / `maestro
policy {show,reload}` would hang forever against a registry/daemon that
is TCP-alive but not making forward progress (SIGSTOP, swap thrash,
deadlock behind slow disk).
Centralize construction in `http::build_client()`, which applies a 5s
connect timeout and a 30s request timeout, and route every call site
through it. Covered by a test that points the client at a peer which
accepts the connection but never responds and asserts the request times
out instead of hanging.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -3,6 +3,7 @@
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::sync::{Arc, Mutex};
|
||||
use std::time::Duration;
|
||||
|
||||
use reqwest::header::{HeaderMap, HeaderValue, COOKIE, SET_COOKIE};
|
||||
use reqwest::{Method, Response, StatusCode};
|
||||
@@ -13,6 +14,30 @@ use crate::config::{CliConfig, ConfigError};
|
||||
|
||||
const COOKIE_NAME: &str = "maestro_session";
|
||||
|
||||
/// Time budget for establishing a TCP+TLS connection to the registry/daemon.
|
||||
const CONNECT_TIMEOUT: Duration = Duration::from_secs(5);
|
||||
/// Total time budget for a single request (connect + send + receive). Bounds
|
||||
/// the case where the peer is TCP-alive but never makes forward progress.
|
||||
const REQUEST_TIMEOUT: Duration = Duration::from_secs(30);
|
||||
|
||||
/// Build a `reqwest::Client` with the CLI's standard timeouts and user-agent.
|
||||
///
|
||||
/// Centralizing construction here guarantees every client the CLI uses gets a
|
||||
/// connect *and* request timeout, so `maestro` never hangs forever on an
|
||||
/// unhealthy registry or daemon (NAP-262).
|
||||
pub fn build_client() -> reqwest::Client {
|
||||
build_client_with_timeouts(CONNECT_TIMEOUT, REQUEST_TIMEOUT)
|
||||
}
|
||||
|
||||
fn build_client_with_timeouts(connect: Duration, request: Duration) -> reqwest::Client {
|
||||
reqwest::Client::builder()
|
||||
.user_agent(concat!("maestro-cli/", env!("CARGO_PKG_VERSION")))
|
||||
.connect_timeout(connect)
|
||||
.timeout(request)
|
||||
.build()
|
||||
.expect("build reqwest client")
|
||||
}
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
pub enum HttpError {
|
||||
#[error("http: {0}")]
|
||||
@@ -42,10 +67,7 @@ pub struct RegistryClient {
|
||||
|
||||
impl RegistryClient {
|
||||
pub fn new(base: String, cookie: Option<String>, config_path: PathBuf) -> Self {
|
||||
let inner = reqwest::Client::builder()
|
||||
.user_agent(concat!("maestro-cli/", env!("CARGO_PKG_VERSION")))
|
||||
.build()
|
||||
.expect("build reqwest client");
|
||||
let inner = build_client();
|
||||
Self {
|
||||
base: base.trim_end_matches('/').to_string(),
|
||||
inner,
|
||||
@@ -171,4 +193,33 @@ mod tests {
|
||||
let raw = "other=1; HttpOnly";
|
||||
assert_eq!(extract_cookie_value(raw, "maestro_session"), None);
|
||||
}
|
||||
|
||||
/// A peer that accepts the TCP connection but never sends a response must
|
||||
/// not hang the client forever: the request timeout has to fire (NAP-262).
|
||||
#[tokio::test]
|
||||
async fn request_timeout_fires_when_peer_never_responds() {
|
||||
use std::io::Read;
|
||||
use std::net::TcpListener;
|
||||
|
||||
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
let addr = listener.local_addr().unwrap();
|
||||
// Accept one connection and sit on it without ever writing a response.
|
||||
let handle = std::thread::spawn(move || {
|
||||
if let Ok((mut sock, _)) = listener.accept() {
|
||||
let mut buf = [0u8; 1024];
|
||||
let _ = sock.read(&mut buf);
|
||||
std::thread::sleep(Duration::from_secs(2));
|
||||
}
|
||||
});
|
||||
|
||||
let client = build_client_with_timeouts(Duration::from_secs(5), Duration::from_millis(300));
|
||||
let err = client
|
||||
.get(format!("http://{addr}/"))
|
||||
.send()
|
||||
.await
|
||||
.expect_err("request should time out, not hang");
|
||||
assert!(err.is_timeout(), "expected timeout error, got: {err}");
|
||||
|
||||
let _ = handle.join();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,7 +36,7 @@ pub struct PolicyView {
|
||||
|
||||
pub async fn run_reload(config_path: &Path, out: &mut impl Write) -> Result<(), PolicyError> {
|
||||
let url = format!("{}/api/policy/reload", local_base_url(config_path)?);
|
||||
let resp = reqwest::Client::new().post(&url).send().await?;
|
||||
let resp = crate::http::build_client().post(&url).send().await?;
|
||||
let status = resp.status();
|
||||
if !status.is_success() {
|
||||
let body = resp.text().await.unwrap_or_default();
|
||||
@@ -65,7 +65,7 @@ pub async fn run_show(
|
||||
out: &mut impl Write,
|
||||
) -> Result<(), PolicyError> {
|
||||
let url = format!("{}/api/policy", local_base_url(config_path)?);
|
||||
let resp = reqwest::Client::new().get(&url).send().await?;
|
||||
let resp = crate::http::build_client().get(&url).send().await?;
|
||||
let status = resp.status();
|
||||
if !status.is_success() {
|
||||
let body = resp.text().await.unwrap_or_default();
|
||||
|
||||
Reference in New Issue
Block a user