From 3fb6b3b4da4a58cd63d96c1804e685bdc254779a Mon Sep 17 00:00:00 2001 From: YoursFunny Date: Thu, 24 Sep 2026 02:52:10 +0800 Subject: [PATCH] =?UTF-8?q?fix(fetch):=20one=20status=20table=20=E2=80=94?= =?UTF-8?q?=20a=20persistent=204xx=20is=20permanent=20everywhere?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit status_error's catch-all called every unlisted status Transient, so a 400/405/418/451 got three retries per link before the same answer (twitter syndication's broken-token 400 being the live example), and download_status_error plus misskey's and bilibili's local fallbacks each carried their own copy of the table — bilibili and misskey classifying a 404 as Transient while the center classified it NotFound. The center now makes any client error except 408/429 a refusal (permanent), the media path delegates to it as status_error("media", ...) and its duplicated fn is deleted, and misskey/bilibili fall through to the center after their own special statuses (misskey's 400 body, bilibili's 412). A table test pins every class. --- crates/x-media/src/site/bilibili/interface.rs | 9 +-- crates/x-media/src/site/download.rs | 21 ++----- crates/x-media/src/site/misskey/interface.rs | 10 ++-- crates/x-media/src/site/mod.rs | 55 ++++++++++++++++--- 4 files changed, 64 insertions(+), 31 deletions(-) diff --git a/crates/x-media/src/site/bilibili/interface.rs b/crates/x-media/src/site/bilibili/interface.rs index fe4c69b..11a9f92 100644 --- a/crates/x-media/src/site/bilibili/interface.rs +++ b/crates/x-media/src/site/bilibili/interface.rs @@ -213,10 +213,11 @@ pub async fn fetch(dynamic_id: &str) -> Result { if !status.is_success() { return Err(match status.as_u16() { 412 => risk_control("412"), - // A refusal or an auth demand is not a bad moment (412 above is - // bilibili's risk control, which does clear on its own). - 401 | 403 => FetchError::Blocked, - _ => FetchError::Transient(format!("bilibili status {status}")), + // Everything else shares the central classes (refusals and gone + // posts permanent, 429/5xx retried). The local fallback used to + // disagree: a bilibili 404 came back Transient here. 412 above is + // bilibili's risk control, which does clear on its own. + _ => crate::site::status_error("bilibili", status), }); } let detail: model::Detail = response.json().await.map_err(|e| FetchError::Site { diff --git a/crates/x-media/src/site/download.rs b/crates/x-media/src/site/download.rs index 8813b92..d619485 100644 --- a/crates/x-media/src/site/download.rs +++ b/crates/x-media/src/site/download.rs @@ -105,7 +105,11 @@ fn download_stalled() -> FetchError { } /// Sends a media-download request: the response head must arrive within the -/// idle window, and a non-2xx status is classified by [`download_status_error`]. +/// idle window, and a non-2xx status is classified by +/// [`super::status_error`] with `"media"` as the name — the same table the +/// site adapters use, so a dead URL and a bad moment read the same everywhere. +/// A transport error never reaches that table — it fails in `send()` and +/// stays [`FetchError::Http`]. async fn send_download(request: reqwest::RequestBuilder) -> Result { let response = match tokio::time::timeout(DOWNLOAD_IDLE_TIMEOUT, request.send()).await { Ok(Ok(response)) => response, @@ -115,7 +119,7 @@ async fn send_download(request: reqwest::RequestBuilder) -> Result reqwe request } -/// Maps a media download's HTTP status onto the same classes the site -/// adapters use, so callers can tell "try again" from "this URL is dead": -/// 4xx is a property of the media (gone, refused by the host), while 429/5xx -/// is a property of the moment. A transport error never reaches this — it -/// fails in `send()` and stays [`FetchError::Http`]. -fn download_status_error(status: reqwest::StatusCode) -> FetchError { - match status.as_u16() { - 401 | 403 => FetchError::Blocked, - 404 | 410 => FetchError::NotFound, - _ => FetchError::Transient(format!("media status {status}")), - } -} - /// Downloads a media file with a hard size cap: the body is streamed and the /// download aborts with [`FetchError::TooLarge`] the moment the cap is /// crossed (or when a declared Content-Length already exceeds it). Keeps the diff --git a/crates/x-media/src/site/misskey/interface.rs b/crates/x-media/src/site/misskey/interface.rs index 458b98f..7c8d9f1 100644 --- a/crates/x-media/src/site/misskey/interface.rs +++ b/crates/x-media/src/site/misskey/interface.rs @@ -51,7 +51,8 @@ pub fn cache_key(url: &str) -> Option { /// Fetches a note from misskey.io by id. The API answers client failures /// with HTTP 400 + `{"error":{"code":...}}` (NO_SUCH_NOTE → NotFound); -/// everything else non-success is transient and retried by [`crate::site::fetch`]. +/// every other non-success status falls through to the shared classes in +/// [`crate::site::status_error`] — persistent 4xx permanent, 429/5xx retried. pub async fn fetch(note_id: &str) -> Result { let response = crate::site::CLIENT .post(API_URL) @@ -62,9 +63,10 @@ pub async fn fetch(note_id: &str) -> Result { if !status.is_success() { return Err(match status.as_u16() { 400 => not_found_or_invalid(response).await, - // A refusal or an auth demand is not a bad moment. - 401 | 403 => FetchError::Blocked, - _ => FetchError::Transient(format!("misskey status {status}")), + // The local fallback used to disagree with the center: a misskey + // 404 came back Transient here and was fetched three more times + // for a note that is simply gone. + _ => crate::site::status_error("misskey", status), }); } response.json().await.map_err(|e| FetchError::Site { diff --git a/crates/x-media/src/site/mod.rs b/crates/x-media/src/site/mod.rs index f689913..e586d68 100644 --- a/crates/x-media/src/site/mod.rs +++ b/crates/x-media/src/site/mod.rs @@ -296,17 +296,18 @@ pub enum FetchError { Io(std::io::Error), } -/// The error class for a non-success HTTP status, as the site adapters that -/// share this mapping classify it: 404/410 mean the post is gone and 401/403 a -/// refusal or an auth demand — both permanent, since retrying cannot change -/// either — while everything else (429, 5xx) is transient and retried by -/// [`fetch`]. `site` only names the adapter in the transient message; a site -/// whose statuses mean something else (bilibili's 412 risk control, misskey's -/// 400 with `NO_SUCH_NOTE`) maps those before falling back here. +/// The error class for a non-success HTTP status, shared by the site +/// adapters, the media downloads and twitter's auth fallback: 404/410 mean +/// the post is gone (permanent), any other client error the source answers +/// on sight is a refusal (permanent too — three retries only delay the same +/// answer), and only 408/429/5xx are a bad moment, retried by [`fetch`]. +/// `site` only names the adapter in the message (`"media"` for downloads); +/// a site whose statuses mean something else (bilibili's 412 risk control, +/// misskey's 400 with `NO_SUCH_NOTE`) maps those before falling back here. pub fn status_error(site: &'static str, status: reqwest::StatusCode) -> FetchError { match status.as_u16() { 404 | 410 => FetchError::NotFound, - 401 | 403 => FetchError::Blocked, + code if status.is_client_error() && !matches!(code, 408 | 429) => FetchError::Blocked, _ => FetchError::Transient(format!("{site} status {status}")), } } @@ -733,6 +734,44 @@ mod tests { } } + #[test] + fn persistent_client_statuses_are_permanent() { + use reqwest::StatusCode; + // The one table every caller shares now: only 408, 429 and 5xx can + // answer differently on a retry. A 400 used to be Transient here and + // in two local fallbacks — twitter syndication's broken-token 400, for + // one, burned three retries per link before saying the same thing. + assert!(matches!( + status_error("x", StatusCode::NOT_FOUND), + FetchError::NotFound + )); + assert!(matches!( + status_error("x", StatusCode::BAD_REQUEST), + FetchError::Blocked + )); + assert!(matches!( + status_error("x", StatusCode::PAYLOAD_TOO_LARGE), + FetchError::Blocked + )); + assert!(matches!( + status_error("x", StatusCode::REQUEST_TIMEOUT), + FetchError::Transient(_) + )); + assert!(matches!( + status_error("x", StatusCode::TOO_MANY_REQUESTS), + FetchError::Transient(_) + )); + assert!(matches!( + status_error("x", StatusCode::INTERNAL_SERVER_ERROR), + FetchError::Transient(_) + )); + // The download path delegates under its own name, same classes. + assert!(matches!( + status_error("media", StatusCode::BAD_REQUEST), + FetchError::Blocked + )); + } + #[tokio::test] async fn disabled_site_is_reported_not_ignored() { // pixiv is the only token-gated site; with PIXIV_REFRESH_TOKEN set it