fix(media-proxy): fail slow external fetches sooner (#3403)

fluxer_media_proxy/src/http_client/mod.rs

@@ -14,10 +14,15 @@ use std::time::Duration;
 
 pub type HttpClient = ClientWithMiddleware;
 
+const EXTERNAL_CONNECT_TIMEOUT_MS: u64 = 3_000;
+const EXTERNAL_READ_TIMEOUT_MS: u64 = 10_000;
+const EXTERNAL_RETRY_WINDOW_MS: u64 = 15_000;
+
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
 pub struct HTTPClientOptions {
     connect_timeout_ms: NonZeroU64,
     request_timeout_ms: NonZeroU64,
+    read_timeout_ms: Option<NonZeroU64>,
     retries: HTTPRetryPolicy,
     address_policy: HTTPAddressPolicy,
 }
@@ -27,6 +32,7 @@ struct HTTPRetryPolicy {
     max_retries: NonZeroU32,
     min_delay_ms: NonZeroU64,
     max_delay_ms: NonZeroU64,
+    window_ms: Option<NonZeroU64>,
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
@@ -44,6 +50,18 @@ impl HTTPClientOptions {
         }
     }
 
+    pub fn external(request_timeout_ms: NonZeroU64) -> Self {
+        let mut options = Self::new(
+            NonZeroU64::new(EXTERNAL_CONNECT_TIMEOUT_MS)
+                .expect("external connection timeout must be nonzero"),
+            request_timeout_ms,
+        )
+        .restrict_to_public();
+        options.read_timeout_ms = NonZeroU64::new(EXTERNAL_READ_TIMEOUT_MS);
+        options.retries.window_ms = NonZeroU64::new(EXTERNAL_RETRY_WINDOW_MS);
+        options
+    }
+
     pub fn restrict_to_public(mut self) -> Self {
         self.address_policy = HTTPAddressPolicy::PublicOnly;
         self
@@ -57,6 +75,7 @@ impl Default for HTTPClientOptions {
                 .expect("default connection timeout must be nonzero"),
             request_timeout_ms: NonZeroU64::new(30_000)
                 .expect("default request timeout must be nonzero"),
+            read_timeout_ms: None,
             retries: HTTPRetryPolicy {
                 max_retries: NonZeroU32::new(2)
                     .expect("default maximum retry count must be nonzero"),
@@ -64,6 +83,7 @@ impl Default for HTTPClientOptions {
                     .expect("default minimum retry delay must be nonzero"),
                 max_delay_ms: NonZeroU64::new(500)
                     .expect("default maximum retry delay must be nonzero"),
+                window_ms: None,
             },
             address_policy: HTTPAddressPolicy::Any,
         }
@@ -80,6 +100,9 @@ pub fn build_raw(options: HTTPClientOptions) -> Result<reqwest::Client, reqwest:
         .timeout(Duration::from_millis(options.request_timeout_ms.get()))
         .redirect(reqwest::redirect::Policy::none())
         .user_agent(crate::constants::OUTBOUND_USER_AGENT);
+    if let Some(read_timeout_ms) = options.read_timeout_ms {
+        builder = builder.read_timeout(Duration::from_millis(read_timeout_ms.get()));
+    }
     if options.address_policy == HTTPAddressPolicy::PublicOnly {
         builder = builder.dns_resolver(Arc::new(PinnedDnsResolver));
     }
@@ -96,6 +119,7 @@ pub fn build(
         max_retries,
         min_delay_ms,
         max_delay_ms,
+        window_ms,
     } = options.retries;
     let retry_policy = ExponentialBackoff::builder()
         .retry_bounds(
@@ -105,7 +129,11 @@ pub fn build(
         .build_with_max_retries(max_retries.get());
     Ok(builder
         .with(RetryTransientMiddleware::new_with_policy_and_strategy(
-            ObservableRetryPolicy::new(retry_policy, Arc::clone(&metrics)),
+            ObservableRetryPolicy::new(
+                retry_policy,
+                window_ms.map(|window| Duration::from_millis(window.get())),
+                Arc::clone(&metrics),
+            ),
             MediaProxyRetryStrategy::new(metrics),
         ))
         .build())
@@ -157,4 +185,24 @@ mod tests {
             .expect_err("the pinned resolver rejects a loopback host");
         assert!(crate::public_net_policy::is_pinned_dns_failure(&error));
     }
+
+    #[test]
+    fn the_external_client_fails_fast_on_slow_hosts() {
+        let options = HTTPClientOptions::external(millis(30_000));
+        assert_eq!(
+            EXTERNAL_CONNECT_TIMEOUT_MS,
+            options.connect_timeout_ms.get()
+        );
+        assert_eq!(30_000, options.request_timeout_ms.get());
+        assert_eq!(
+            Some(EXTERNAL_READ_TIMEOUT_MS),
+            options.read_timeout_ms.map(NonZeroU64::get)
+        );
+        assert_eq!(
+            Some(EXTERNAL_RETRY_WINDOW_MS),
+            options.retries.window_ms.map(NonZeroU64::get)
+        );
+        assert_eq!(HTTPAddressPolicy::PublicOnly, options.address_policy);
+        build_raw(options).expect("the external client configuration is valid");
+    }
 }

fluxer_media_proxy/src/http_client/retry.rs

@@ -8,22 +8,40 @@ use reqwest_retry::{
 };
 use std::io::ErrorKind;
 use std::sync::Arc;
-use std::time::SystemTime;
+use std::time::{Duration, SystemTime};
 
 pub(super) struct ObservableRetryPolicy {
     inner: ExponentialBackoff,
+    retry_window: Option<Duration>,
     metrics: Arc<HTTPClientMetrics>,
 }
 
 impl ObservableRetryPolicy {
-    pub(super) fn new(inner: ExponentialBackoff, metrics: Arc<HTTPClientMetrics>) -> Self {
-        Self { inner, metrics }
+    pub(super) fn new(
+        inner: ExponentialBackoff,
+        retry_window: Option<Duration>,
+        metrics: Arc<HTTPClientMetrics>,
+    ) -> Self {
+        Self {
+            inner,
+            retry_window,
+            metrics,
+        }
     }
 }
 
 impl RetryPolicy for ObservableRetryPolicy {
     fn should_retry(&self, request_start_time: SystemTime, n_past_retries: u32) -> RetryDecision {
-        let decision = self.inner.should_retry(request_start_time, n_past_retries);
+        let window_spent = self.retry_window.is_some_and(|window| {
+            request_start_time
+                .elapsed()
+                .is_ok_and(|elapsed| elapsed >= window)
+        });
+        let decision = if window_spent {
+            RetryDecision::DoNotRetry
+        } else {
+            self.inner.should_retry(request_start_time, n_past_retries)
+        };
         match decision {
             RetryDecision::Retry { .. } => self.metrics.record_retry(),
             RetryDecision::DoNotRetry => self.metrics.record_retries_exhausted(),
@@ -263,7 +281,7 @@ mod tests {
         let backoff = ExponentialBackoff::builder()
             .retry_bounds(Duration::from_millis(1), Duration::from_millis(2))
             .build_with_max_retries(1);
-        let policy = ObservableRetryPolicy::new(backoff, metrics.http_client());
+        let policy = ObservableRetryPolicy::new(backoff, None, metrics.http_client());
         let started = SystemTime::now();
         assert!(matches!(
             policy.should_retry(started, 0),
@@ -329,4 +347,38 @@ mod tests {
             Some(Retryable::Transient)
         ));
     }
+
+    fn policy(window: Option<Duration>) -> ObservableRetryPolicy {
+        ObservableRetryPolicy::new(
+            ExponentialBackoff::builder().build_with_max_retries(2),
+            window,
+            Arc::new(HTTPClientMetrics::new()),
+        )
+    }
+
+    #[test]
+    fn a_request_past_its_retry_window_is_not_retried() {
+        let started = SystemTime::now() - Duration::from_secs(20);
+        assert!(matches!(
+            policy(Some(Duration::from_secs(15))).should_retry(started, 0),
+            RetryDecision::DoNotRetry
+        ));
+    }
+
+    #[test]
+    fn a_request_inside_its_retry_window_is_retried() {
+        assert!(matches!(
+            policy(Some(Duration::from_secs(15))).should_retry(SystemTime::now(), 0),
+            RetryDecision::Retry { .. }
+        ));
+    }
+
+    #[test]
+    fn a_policy_without_a_window_keeps_retrying_slow_requests() {
+        let started = SystemTime::now() - Duration::from_secs(60);
+        assert!(matches!(
+            policy(None).should_retry(started, 0),
+            RetryDecision::Retry { .. }
+        ));
+    }
 }

fluxer_media_proxy/src/server/state.rs

@@ -42,7 +42,7 @@ impl AppState {
             NonZeroU64::new(cfg.socket_io_timeout_ms).unwrap_or(NonZeroU64::MIN);
         let store = Store::try_new(cfg.clone(), metrics.storage(), metrics.http_client())?;
         let external_client = http_client::build(
-            HTTPClientOptions::new(socket_io_timeout, socket_io_timeout).restrict_to_public(),
+            HTTPClientOptions::external(socket_io_timeout),
             metrics.http_client(),
         )?;
         let nsfw = NSFWClient::new(