From e65b97e23922ff0a9d45d981a1dea3f1b8968740 Mon Sep 17 00:00:00 2001 From: Michael Anderson Date: Wed, 14 Oct 2026 13:08:15 -0500 Subject: [PATCH] Model downloads: Time out and cancel the name lookup (#1532) The host name lookup blocked with no deadline before the cancel check, and each address then got the full connect timeout. The lookup now runs on a helper thread under the connect deadline, which all addresses share. --- crates/fetch/src/http.rs | 124 ++++++++++++++++++++++++++++++++------- docs/whats-new.md | 5 ++ 2 files changed, 109 insertions(+), 20 deletions(-) diff --git a/crates/fetch/src/http.rs b/crates/fetch/src/http.rs index c2b5bee1e..98eee913b 100644 --- a/crates/fetch/src/http.rs +++ b/crates/fetch/src/http.rs @@ -10,7 +10,7 @@ //! the downloading thread. No proxies (see docs/ai-masks.md). use std::io::{ErrorKind, Read, Write}; -use std::net::{TcpStream, ToSocketAddrs}; +use std::net::{SocketAddr, TcpStream, ToSocketAddrs}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use std::time::{Duration, Instant}; @@ -182,27 +182,68 @@ fn tls_config() -> Result, HttpError> { Ok(Arc::new(config)) } +/// Resolve a host name within `deadline`, checking the cancel flag while it runs. The system resolver blocks with no +/// timeout of its own, so it runs on a helper thread (which ends when the resolver returns); a lookup that takes too +/// long is given up on, and Cancel works during it (issue #1532). +fn lookup( + host: &str, + resolve: impl FnOnce() -> std::io::Result> + Send + 'static, + deadline: Instant, + limits: &Limits, +) -> Result, HttpError> { + if limits.cancel.load(Ordering::Relaxed) { + return Err(HttpError::Cancelled); + } + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::Builder::new() + .name("lc-fetch-lookup".into()) + .spawn(move || { + let _ = tx.send(resolve()); + }) + .map_err(|e| HttpError::Connect(format!("{host}: {e}")))?; + loop { + if limits.cancel.load(Ordering::Relaxed) { + return Err(HttpError::Cancelled); + } + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return Err(HttpError::Connect(format!("{host}: the name lookup timed out"))); + } + match rx.recv_timeout(left.min(TICK)) { + Ok(r) => return r.map_err(|e| HttpError::Connect(format!("{host}: {e}"))), + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {} + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => return Err(HttpError::Connect(format!("{host}: the name lookup failed"))), + } + } +} + +/// Connect to the first of `addrs` that answers, all of them together within `deadline` (each used to get the whole +/// connect timeout of its own, issue #1532). +fn connect_any(host: &str, addrs: &[SocketAddr], deadline: Instant, limits: &Limits) -> Result { + let mut last = format!("{host}: no address"); + for a in addrs { + if limits.cancel.load(Ordering::Relaxed) { + return Err(HttpError::Cancelled); + } + let left = deadline.saturating_duration_since(Instant::now()); + if left.is_zero() { + return Err(HttpError::Connect(format!("{host}: connecting timed out"))); + } + match TcpStream::connect_timeout(a, left) { + Ok(s) => return Ok(s), + Err(e) => last = format!("{host}: {e}"), + } + } + Err(HttpError::Connect(last)) +} + impl Stream { fn connect(url: &Url, limits: &Limits) -> Result { - let addrs: Vec<_> = (url.host.as_str(), url.port).to_socket_addrs().map_err(|e| HttpError::Connect(format!("{}: {e}", url.host)))?.collect(); - if addrs.is_empty() { - return Err(HttpError::Connect(format!("{}: no address", url.host))); - } - let mut last = String::new(); - let mut sock = None; - for a in addrs { - if limits.cancel.load(Ordering::Relaxed) { - return Err(HttpError::Cancelled); - } - match TcpStream::connect_timeout(&a, limits.connect) { - Ok(s) => { - sock = Some(s); - break; - } - Err(e) => last = format!("{}: {e}", url.host), - } - } - let sock = sock.ok_or(HttpError::Connect(last))?; + // the lookup and the connection attempts share one connect timeout + let deadline = Instant::now() + limits.connect; + let (host, port) = (url.host.clone(), url.port); + let addrs = lookup(&url.host, move || (host.as_str(), port).to_socket_addrs().map(Iterator::collect), deadline, limits)?; + let sock = connect_any(&url.host, &addrs, deadline, limits)?; sock.set_read_timeout(Some(TICK)).map_err(|e| HttpError::Io(e.to_string()))?; sock.set_write_timeout(Some(TICK)).map_err(|e| HttpError::Io(e.to_string()))?; let _ = sock.set_nodelay(true); @@ -512,6 +553,49 @@ pub fn get(url: &Url, headers: &[(&str, String)], limits: &Limits) -> Result Limits<'_> { + Limits { connect: Duration::from_millis(300), stall: Duration::from_secs(1), cancel } + } + + /// A name lookup that hangs is given up on at the connect timeout, and Cancel stops it at once (issue #1532). + #[test] + fn a_hung_name_lookup_times_out_and_can_be_cancelled() { + let cancel = AtomicBool::new(false); + let slow = || { + std::thread::sleep(Duration::from_secs(10)); + Ok(Vec::new()) + }; + let t = Instant::now(); + let r = lookup("slow.example", slow, Instant::now() + Duration::from_millis(300), &limits(&cancel)); + assert!(matches!(&r, Err(HttpError::Connect(m)) if m.contains("timed out")), "{r:?}"); + assert!(t.elapsed() < Duration::from_secs(2), "{:?}", t.elapsed()); + cancel.store(true, Ordering::Relaxed); + let t = Instant::now(); + assert!(matches!(lookup("slow.example", slow, Instant::now() + Duration::from_secs(30), &limits(&cancel)), Err(HttpError::Cancelled))); + assert!(t.elapsed() < Duration::from_secs(1)); + // a lookup that answers is used + let cancel = AtomicBool::new(false); + let local: SocketAddr = "127.0.0.1:9".parse().unwrap(); + assert_eq!(lookup("local", move || Ok(vec![local]), Instant::now() + Duration::from_secs(5), &limits(&cancel)).unwrap(), [local]); + } + + /// Several addresses that don't answer share one deadline instead of each getting the full timeout. + #[test] + fn addresses_share_one_connect_deadline() { + let cancel = AtomicBool::new(false); + // non-routable addresses: each either hangs until the deadline or fails at once, never longer + let addrs: Vec = (1..=5).map(|i| format!("10.255.255.{i}:81").parse().unwrap()).collect(); + let t = Instant::now(); + let r = connect_any("blackhole", &addrs, Instant::now() + Duration::from_millis(400), &limits(&cancel)); + assert!(r.is_err()); + assert!(t.elapsed() < Duration::from_millis(1500), "{:?}", t.elapsed()); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/docs/whats-new.md b/docs/whats-new.md index 1bf44c3f4..3ca956764 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -2,6 +2,11 @@ ## 2026-10-14 +### Reliability +- Model downloads (AI denoise, faces, SAM) give up on a server name that doesn't resolve within the connect timeout, + and Cancel stops a download that is still looking the name up (issue #1532). The lookup used to block with no time + limit, and each of a server's addresses then got the whole connect timeout of its own. + ### Hard-edged Heal and Clone spots - A spot with Feather at 0 no longer draws black pixels exactly on its radius. This also fixes spots at 0% Opacity changing pixels along their edge. The hard edge now leaves pixels on the radius unchanged.