From 6104c68768b6792a5f5900999d7fe517719a5293 Mon Sep 17 00:00:00 2001 From: timedout Date: Tue, 11 Aug 2026 16:57:51 +0100 Subject: [PATCH] style: Centralise decision for stale marker --- src/core/error/mod.rs | 4 ++++ src/service/federation/execute.rs | 15 +++++++++++++-- src/service/federation/mod.rs | 16 +++++++++++++++- src/service/sending/sender.rs | 4 +--- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/core/error/mod.rs b/src/core/error/mod.rs index a4e8c40aa..ad3040fff 100644 --- a/src/core/error/mod.rs +++ b/src/core/error/mod.rs @@ -287,3 +287,7 @@ impl From for FormattedReqwestError { impl From for Error { fn from(err: reqwest::Error) -> Self { Self::Reqwest(err.into()) } } + +impl From for reqwest::Error { + fn from(val: FormattedReqwestError) -> Self { val.0 } +} diff --git a/src/service/federation/execute.rs b/src/service/federation/execute.rs index fb0f56c9f..bd633195d 100644 --- a/src/service/federation/execute.rs +++ b/src/service/federation/execute.rs @@ -186,12 +186,23 @@ impl super::Service { self.handle_response::(dest, actual, &method, &url, response) .await, | Err(error) => { - if error.is_connect() { + // This awful wrapping hack is required because `reqwest::Error` does not + // implement `Clone`, but we need to convert it into a local error type to pass + // to should_mark_stale, but handle_error then itself expects the original + // reqwest error. + // handle_error *could* just take the wrapped error, but it'd have to unwrap it + // anyway. + let wrapped = Error::Reqwest(error.into()); + if self.should_mark_stale(&wrapped) { debug_info!("{dest} is unhealthy & stale due to a connect error"); self.mark_destination_stale(dest); self.hit_unhealthy(dest.to_owned()); } - Err(handle_error(actual, &method, &url, error).expect_err("always returns error")) + let Error::Reqwest(unwrapped) = wrapped else { + unreachable!("wrapped reqwest error must unwrap to a reqwest error"); + }; + Err(handle_error(actual, &method, &url, unwrapped.into()) + .expect_err("always returns error")) }, } } diff --git a/src/service/federation/mod.rs b/src/service/federation/mod.rs index 6beba9275..acec86919 100644 --- a/src/service/federation/mod.rs +++ b/src/service/federation/mod.rs @@ -151,7 +151,9 @@ impl Service { retry_after: Some(RetryAfter::Delay(retry_after)), })), format!( - "Remote server is currently unhealthy (not retrying for another {} seconds)", + "Remote server {} is currently unhealthy (not retrying for another {} \ + seconds)", + server_name, retry_after.as_secs() ) .into(), @@ -173,6 +175,18 @@ impl Service { .insert(server_name.to_owned()); } + /// Determines whether a destination should be marked "stale" depending on + /// the returned error. + pub fn should_mark_stale(&self, error: &Error) -> bool { + if let Error::Reqwest(error) = error { + if error.is_connect() { + return true; + } + } + + error.status_code() == StatusCode::MISDIRECTED_REQUEST + } + /// Returns a clone of the internal remote health tracking map. pub fn remote_health(&self) -> HashMap { self.remote_health.read().clone() diff --git a/src/service/sending/sender.rs b/src/service/sending/sender.rs index 3711f2fcf..cd55bc084 100644 --- a/src/service/sending/sender.rs +++ b/src/service/sending/sender.rs @@ -145,9 +145,7 @@ impl Service { | Ok(dest) => self.handle_response_ok(&dest, futures, statuses).await, | Err((dest, e)) => { if let Destination::Federation(dest) = &dest { - if let Error::Reqwest(e) = &e - && e.is_connect() - { + if self.services.federation.should_mark_stale(&e) { debug!("{dest} is now unhealthy & stale due to a connect error: {e:?}"); self.services.federation.mark_destination_stale(dest); self.services.federation.hit_unhealthy(dest.clone());