repo-stats: retry on non-standard HTTP 499 too

Author: syphar

Cargo.lock

@@ -34,6 +34,7 @@ docs_rs_test_fakes = { path = "../docs_rs_test_fakes" }
 docs_rs_types = { path = "../docs_rs_types", features = ["testing"] }
 mockito = { workspace = true }
 pretty_assertions = { workspace = true }
+test-case = { workspace = true }
 tokio = { workspace = true }
 
 [lints]

crates/lib/docs_rs_repository_stats/Cargo.toml

@@ -1,7 +1,7 @@
 use crate::{
     RateLimitReached,
     config::Config,
-    retry::NoRateLimitRetryStrategy,
+    retry::RepositoryForgeRetryStrategy,
     updater::{FetchRepositoriesResult, Repository, RepositoryForge, RepositoryName},
 };
 use anyhow::{Result, anyhow, bail};
@@ -84,7 +84,7 @@ impl GitHub {
         )
         .with(RetryTransientMiddleware::new_with_policy_and_strategy(
             ExponentialBackoff::builder().build_with_max_retries(config.github_api_retries),
-            NoRateLimitRetryStrategy,
+            RepositoryForgeRetryStrategy,
         ))
         .build();
 
@@ -309,6 +309,7 @@ mod tests {
     use anyhow::Result;
     use docs_rs_config::AppConfig as _;
     use reqwest::header::AUTHORIZATION;
+    use test_case::test_case;
 
     const TEST_TOKEN: &str = "qsjdnfqdq";
 
@@ -426,7 +427,9 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn retries_server_errors() -> Result<()> {
+    #[test_case(499)]
+    #[test_case(500)]
+    async fn retries_server_errors(status_code: usize) -> Result<()> {
         const RETRIES: u32 = 2;
 
         let mut config = github_config()?;
@@ -436,7 +439,7 @@ mod tests {
         let mock = server
             .mock("POST", "/graphql")
             .with_header("content-type", "application/json")
-            .with_status(500)
+            .with_status(status_code)
             .expect((RETRIES + 1) as usize)
             .create();
 
@@ -447,7 +450,7 @@ mod tests {
             .await
             .unwrap_err();
 
-        assert!(err.to_string().contains("500 Internal Server Error"));
+        assert!(err.to_string().contains(&status_code.to_string()));
         mock.assert();
 
         Ok(())

crates/lib/docs_rs_repository_stats/src/github.rs

@@ -12,7 +12,7 @@ use tracing::warn;
 
 use crate::{
     RateLimitReached,
-    retry::NoRateLimitRetryStrategy,
+    retry::RepositoryForgeRetryStrategy,
     updater::{FetchRepositoriesResult, Repository, RepositoryForge, RepositoryName},
 };
 
@@ -88,7 +88,7 @@ impl GitLab {
         )
         .with(RetryTransientMiddleware::new_with_policy_and_strategy(
             ExponentialBackoff::builder().build_with_max_retries(api_retries),
-            NoRateLimitRetryStrategy,
+            RepositoryForgeRetryStrategy,
         ))
         .build();
 

crates/lib/docs_rs_repository_stats/src/gitlab.rs

@@ -2,20 +2,32 @@ use reqwest::StatusCode;
 use reqwest_middleware::Error as MiddlewareError;
 use reqwest_retry::{DefaultRetryableStrategy, Retryable, RetryableStrategy};
 
-/// Retries transient failures, except rate limits, which callers must handle immediately.
+/// Retry policy for repository-forge APIs.
 ///
 /// Repo-Stats run as a scheduled task once an hour. When we reach a rate limit
 /// we just stop handling repos for this run, and continue at the next scheduled time.
-pub(crate) struct NoRateLimitRetryStrategy;
+///
+/// Also, some HTTP statuses are treated as fatal in `DefaultRetryableStrategy`, while
+/// we think they are actually transient.
+pub(crate) struct RepositoryForgeRetryStrategy;
 
-impl RetryableStrategy for NoRateLimitRetryStrategy {
+impl RetryableStrategy for RepositoryForgeRetryStrategy {
     fn handle(&self, result: &Result<reqwest::Response, MiddlewareError>) -> Option<Retryable> {
-        if let Ok(response) = result
-            && response.status() == StatusCode::TOO_MANY_REQUESTS
-        {
-            Some(Retryable::Fatal)
-        } else {
-            DefaultRetryableStrategy.handle(result)
+        match result {
+            Ok(response) if response.status() == StatusCode::TOO_MANY_REQUESTS => {
+                Some(Retryable::Fatal)
+            }
+            Ok(response) if response.status().as_u16() == 499 => {
+                // NGINX defines a non-standard HTTP status code:
+                // `NGX_HTTP_CLIENT_CLOSED_REQUEST     499`
+                // See:
+                // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#nginx
+                // https://web.archive.org/web/20170919111558/http://lxr.nginx.org/source/src/http/ngx_http_request.h
+                //
+                // We believe retrying is safe because the server did not complete the request.
+                Some(Retryable::Transient)
+            }
+            _ => DefaultRetryableStrategy.handle(result),
         }
     }
 }