diff --git a/crates/ipx/CHANGELOG.md b/crates/ipx/CHANGELOG.md index b1d7d92..eb86206 100644 --- a/crates/ipx/CHANGELOG.md +++ b/crates/ipx/CHANGELOG.md @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Artwork that cannot be shown is asked for again a day later rather than every hour: one too large is refused on its stated size before it is read, and a refusal from the publisher or one ipx makes (too large, not an image) is remembered for a day; JPEG XL pictures sent without a type are shown (#448). - `ipx reap --dry-run` says "would delete" of each file and of the total, rather than "deleted", word for word a real reap, of files it kept (#461). - Subscribing to a feed someone here already reads puts its recent items to your Signals at once, rather than at the judge's next pass, up to five minutes later (#415). - A download a restart stopped, a deploy's for one, is taken up again as the daemon starts, rather than waiting for its feed's next check, an hour or a day away (#410). diff --git a/crates/ipx/src/art.rs b/crates/ipx/src/art.rs index bc90ef2..cdde7c2 100644 --- a/crates/ipx/src/art.rs +++ b/crates/ipx/src/art.rs @@ -92,6 +92,10 @@ pub fn write(file: &Path, kind: &str, body: &[u8]) -> std::io::Result<()> { /// How long an address that could not be fetched is answered from memory before it is tried again. const RETRY: Duration = Duration::from_secs(60 * 60); +/// The same for a failure the publisher will not change by morning: too large, not an image, a +/// 4xx. At an hour, five GIFs over the limit were read 20 MB at a time 37 times each in five days, +/// about 3 GB for nothing (#448). +const RETRY_LASTING: Duration = Duration::from_secs(24 * 60 * 60); /// Addresses that could not be fetched lately, and why. Without it a page shown again fetched /// them again: five covers too big to keep were downloaded from Patreon on every load (#169). @@ -99,7 +103,7 @@ const RETRY: Duration = Duration::from_secs(60 * 60); // one per artwork address a feed or item names; move it to disk or prune it on trim if that grows. static FAILED: LazyLock>> = LazyLock::new(Default::default); -/// Why `url` could not be fetched, if it failed less than an hour ago. +/// Why `url` could not be fetched, if it failed lately: an hour, or a day for a lasting failure. pub fn failed(url: &str) -> Option { failed_as_of(url, Instant::now()) } @@ -107,7 +111,7 @@ pub fn failed(url: &str) -> Option { fn failed_as_of(url: &str, now: Instant) -> Option { let mut map = FAILED.lock().unwrap_or_else(|e| e.into_inner()); match map.get(url) { - Some((at, why)) if now.duration_since(*at) < RETRY => Some(why.clone()), + Some((until, why)) if now < *until => Some(why.clone()), Some(_) => { map.remove(url); None @@ -116,8 +120,9 @@ fn failed_as_of(url: &str, now: Instant) -> Option { } } -pub fn fail(url: &str, why: &str) { - FAILED.lock().unwrap_or_else(|e| e.into_inner()).insert(url.to_owned(), (Instant::now(), why.to_owned())); +pub fn fail(url: &str, why: &str, lasting: bool) { + let until = Instant::now() + if lasting { RETRY_LASTING } else { RETRY }; + FAILED.lock().unwrap_or_else(|e| e.into_inner()).insert(url.to_owned(), (until, why.to_owned())); } /// Removes the least recently used images until what is kept fits in `limit` bytes; 0 empties it. @@ -184,13 +189,16 @@ mod tests { } #[test] - fn a_failure_is_remembered_for_an_hour() { + fn a_failure_is_remembered_for_an_hour_and_a_lasting_one_for_a_day() { let u = "https://example.com/a-failure-is-remembered.png"; assert_eq!(failed(u), None); - fail(u, "larger than 5 MB"); - assert_eq!(failed(u).as_deref(), Some("larger than 5 MB")); + fail(u, "timed out", false); + assert_eq!(failed(u).as_deref(), Some("timed out")); assert_eq!(failed_as_of(u, Instant::now() + RETRY), None); // Asked for after the hour, it is forgotten, not just ignored. assert_eq!(failed(u), None); + fail(u, "larger than 20 MB", true); + assert_eq!(failed_as_of(u, Instant::now() + RETRY).as_deref(), Some("larger than 20 MB")); + assert_eq!(failed_as_of(u, Instant::now() + RETRY_LASTING), None); } } diff --git a/crates/ipx/src/web.rs b/crates/ipx/src/web.rs index 6a657dd..29b2ff9 100644 --- a/crates/ipx/src/web.rs +++ b/crates/ipx/src/web.rs @@ -1777,12 +1777,36 @@ mod tests { assert_eq!(image_kind("application/octet-stream", &jpeg).as_deref(), Some("image/jpeg")); assert_eq!(image_kind("", b"\x89PNG\r\n").as_deref(), Some("image/png")); assert_eq!(image_kind("", b"RIFF\0\0\0\0WEBPVP8").as_deref(), Some("image/webp")); + assert_eq!(image_kind("application/octet-stream", b"\xff\x0a\xfa").as_deref(), Some("image/jxl")); + assert_eq!(image_kind("", b"\0\0\0\x0cJXL \r\n\x87\n\0").as_deref(), Some("image/jxl")); assert_eq!(image_kind("image/avif", b"").as_deref(), Some("image/avif")); // A page, or an untyped SVG, stays refused. assert_eq!(image_kind("text/html", &jpeg), None); assert_eq!(image_kind("", b""), None); } + #[tokio::test] + async fn an_artwork_failure_lasts_when_the_publisher_or_ipx_refuses_it() { + // Answers each request with the status its path names. + let l = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let at = l.local_addr().unwrap(); + tokio::spawn(async move { + use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _}; + while let Ok((mut s, _)) = l.accept().await { + let mut buf = [0; 1024]; + let n = s.read(&mut buf).await.unwrap_or(0); + let code = String::from_utf8_lossy(&buf[..n]).split('/').nth(1).unwrap_or("500")[..3].to_owned(); + let _ = s.write_all(format!("HTTP/1.1 {code} X\r\ncontent-length: 0\r\n\r\n").as_bytes()).await; + } + }); + let c = reqwest::Client::new(); + for (code, want) in [(404, true), (410, true), (408, false), (429, false), (503, false)] { + let e = c.get(format!("http://{at}/{code}")).send().await.unwrap().error_for_status().unwrap_err(); + assert_eq!(lasting(&e.into()), want, "{code}"); + } + assert!(lasting(&anyhow::anyhow!("larger than 20971520 bytes"))); + } + #[test] fn a_signal_icon_is_a_style_and_a_plain_name() { assert_eq!(signal_icon(Some("solid/robot".into())).ok().flatten().as_deref(), Some("solid/robot")); @@ -2804,6 +2828,8 @@ async fn art(State(state): State, Query(q): Query) -> Respon let mut r = state.ctx.client.get(&q.u).timeout(crate::feed::FEED_TIMEOUT).send().await?.error_for_status()?; let said = r.headers().get(header::CONTENT_TYPE).and_then(|v| v.to_str().ok()).unwrap_or("").to_owned(); anyhow::ensure!(said.starts_with("image/") || untyped(&said), "not an image: {said}"); + // Said up front, refused up front: read chunk by chunk, each was 20 MB fetched to be thrown away (#448). + anyhow::ensure!(r.content_length().is_none_or(|n| n <= MAX as u64), "larger than {MAX} bytes"); let mut body = Vec::new(); while let Some(c) = r.chunk().await? { body.extend_from_slice(&c); @@ -2830,12 +2856,25 @@ async fn art(State(state): State, Query(q): Query) -> Respon let why = format!("{e:#}"); // The access line says only that it failed, not which image or why (#169). tracing::warn!(url = %crate::feed::redact(&q.u), error = %crate::feed::redact_text(&why), "could not fetch artwork"); - crate::art::fail(&q.u, &why); + crate::art::fail(&q.u, &why, lasting(&e)); (StatusCode::BAD_GATEWAY, why).into_response() } } } +/// Whether an artwork fetch failed for a reason that will still hold tomorrow: refused by the +/// publisher (a 4xx but a timeout or too many requests), or what ipx refuses (too large, not an +/// image), which are its own errors and not reqwest's. A timeout, a 5xx or a dropped connection +/// may be gone in an hour. +fn lasting(e: &anyhow::Error) -> bool { + match e.downcast_ref::() { + Some(re) => re.status().is_some_and(|s| { + s.is_client_error() && s != StatusCode::REQUEST_TIMEOUT && s != StatusCode::TOO_MANY_REQUESTS + }), + None => true, + } +} + /// The fetches of artwork under way, by address, each a lock the requests for it take in turn. static ART_FETCHES: std::sync::LazyLock< std::sync::Mutex>>>, @@ -2884,6 +2923,8 @@ fn image_kind(said: &str, body: &[u8]) -> Option { [0x89, b'P', b'N', b'G', ..] => "image/png", [b'G', b'I', b'F', b'8', ..] => "image/gif", [b'R', b'I', b'F', b'F', _, _, _, _, b'W', b'E', b'B', b'P', ..] => "image/webp", + // JPEG XL, bare or in its box, which shkspr.mobi sends as octet-stream (#448); Safari shows it. + [0xFF, 0x0A, ..] | [0, 0, 0, 0x0C, b'J', b'X', b'L', b' ', 0x0D, 0x0A, 0x87, 0x0A, ..] => "image/jxl", _ => return None, }; Some(kind.to_owned())