Artwork that cannot be shown is not fetched again every hour (#448)
/api/art remembered a failed image for an hour, whatever the failure, so an image that could never pass was fetched again on the next view after that. In 2026-11-09..13, five images over the 20 MB limit were fetched 146 times, each read 20 MB chunk by chunk before being refused, about 3 GB for nothing; a 400 from b2c-contenthub.com was asked for 34 times. Now a Content-Length over the limit is refused before the body is read, and a failure that will still hold tomorrow is remembered for a day: a 4xx other than 408 and 429, or ipx's own refusal (too large, not an image). Timeouts, 5xx and dropped connections still retry in an hour. JPEG XL, which shkspr.mobi sends as application/octet-stream, was refused as not an image; its two signatures are sniffed now, and Safari shows it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
|
|
||||||
### Fixed
|
### 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).
|
- `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).
|
- 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).
|
- 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).
|
||||||
|
|||||||
@@ -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.
|
/// 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);
|
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
|
/// 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).
|
/// 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.
|
// one per artwork address a feed or item names; move it to disk or prune it on trim if that grows.
|
||||||
static FAILED: LazyLock<Mutex<HashMap<String, (Instant, String)>>> = LazyLock::new(Default::default);
|
static FAILED: LazyLock<Mutex<HashMap<String, (Instant, String)>>> = 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<String> {
|
pub fn failed(url: &str) -> Option<String> {
|
||||||
failed_as_of(url, Instant::now())
|
failed_as_of(url, Instant::now())
|
||||||
}
|
}
|
||||||
@@ -107,7 +111,7 @@ pub fn failed(url: &str) -> Option<String> {
|
|||||||
fn failed_as_of(url: &str, now: Instant) -> Option<String> {
|
fn failed_as_of(url: &str, now: Instant) -> Option<String> {
|
||||||
let mut map = FAILED.lock().unwrap_or_else(|e| e.into_inner());
|
let mut map = FAILED.lock().unwrap_or_else(|e| e.into_inner());
|
||||||
match map.get(url) {
|
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(_) => {
|
Some(_) => {
|
||||||
map.remove(url);
|
map.remove(url);
|
||||||
None
|
None
|
||||||
@@ -116,8 +120,9 @@ fn failed_as_of(url: &str, now: Instant) -> Option<String> {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn fail(url: &str, why: &str) {
|
pub fn fail(url: &str, why: &str, lasting: bool) {
|
||||||
FAILED.lock().unwrap_or_else(|e| e.into_inner()).insert(url.to_owned(), (Instant::now(), why.to_owned()));
|
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.
|
/// Removes the least recently used images until what is kept fits in `limit` bytes; 0 empties it.
|
||||||
@@ -184,13 +189,16 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[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";
|
let u = "https://example.com/a-failure-is-remembered.png";
|
||||||
assert_eq!(failed(u), None);
|
assert_eq!(failed(u), None);
|
||||||
fail(u, "larger than 5 MB");
|
fail(u, "timed out", false);
|
||||||
assert_eq!(failed(u).as_deref(), Some("larger than 5 MB"));
|
assert_eq!(failed(u).as_deref(), Some("timed out"));
|
||||||
assert_eq!(failed_as_of(u, Instant::now() + RETRY), None);
|
assert_eq!(failed_as_of(u, Instant::now() + RETRY), None);
|
||||||
// Asked for after the hour, it is forgotten, not just ignored.
|
// Asked for after the hour, it is forgotten, not just ignored.
|
||||||
assert_eq!(failed(u), None);
|
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);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1777,12 +1777,36 @@ mod tests {
|
|||||||
assert_eq!(image_kind("application/octet-stream", &jpeg).as_deref(), Some("image/jpeg"));
|
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"\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("", 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"));
|
assert_eq!(image_kind("image/avif", b"").as_deref(), Some("image/avif"));
|
||||||
// A page, or an untyped SVG, stays refused.
|
// A page, or an untyped SVG, stays refused.
|
||||||
assert_eq!(image_kind("text/html", &jpeg), None);
|
assert_eq!(image_kind("text/html", &jpeg), None);
|
||||||
assert_eq!(image_kind("", b"<svg onload=alert(1)>"), None);
|
assert_eq!(image_kind("", b"<svg onload=alert(1)>"), 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]
|
#[test]
|
||||||
fn a_signal_icon_is_a_style_and_a_plain_name() {
|
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"));
|
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<WebState>, Query(q): Query<ArtQuery>) -> Respon
|
|||||||
let mut r = state.ctx.client.get(&q.u).timeout(crate::feed::FEED_TIMEOUT).send().await?.error_for_status()?;
|
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();
|
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}");
|
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();
|
let mut body = Vec::new();
|
||||||
while let Some(c) = r.chunk().await? {
|
while let Some(c) = r.chunk().await? {
|
||||||
body.extend_from_slice(&c);
|
body.extend_from_slice(&c);
|
||||||
@@ -2830,12 +2856,25 @@ async fn art(State(state): State<WebState>, Query(q): Query<ArtQuery>) -> Respon
|
|||||||
let why = format!("{e:#}");
|
let why = format!("{e:#}");
|
||||||
// The access line says only that it failed, not which image or why (#169).
|
// 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");
|
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()
|
(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::<reqwest::Error>() {
|
||||||
|
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.
|
/// The fetches of artwork under way, by address, each a lock the requests for it take in turn.
|
||||||
static ART_FETCHES: std::sync::LazyLock<
|
static ART_FETCHES: std::sync::LazyLock<
|
||||||
std::sync::Mutex<std::collections::HashMap<String, Arc<tokio::sync::Mutex<()>>>>,
|
std::sync::Mutex<std::collections::HashMap<String, Arc<tokio::sync::Mutex<()>>>>,
|
||||||
@@ -2884,6 +2923,8 @@ fn image_kind(said: &str, body: &[u8]) -> Option<String> {
|
|||||||
[0x89, b'P', b'N', b'G', ..] => "image/png",
|
[0x89, b'P', b'N', b'G', ..] => "image/png",
|
||||||
[b'G', b'I', b'F', b'8', ..] => "image/gif",
|
[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",
|
[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,
|
_ => return None,
|
||||||
};
|
};
|
||||||
Some(kind.to_owned())
|
Some(kind.to_owned())
|
||||||
|
|||||||
Reference in New Issue
Block a user