From 0c748146b08ffe84d83a7b0a6ed9d8c058e52907 Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Sat, 12 Sep 2026 06:52:08 +0200 Subject: [PATCH] repo-stats: retry on non-standard HTTP 499 too --- Cargo.lock | 1 + .../lib/docs_rs_repository_stats/Cargo.toml | 1 + .../docs_rs_repository_stats/src/github.rs | 13 ++++---- .../docs_rs_repository_stats/src/gitlab.rs | 4 +-- .../lib/docs_rs_repository_stats/src/retry.rs | 30 +++++++++++++------ 5 files changed, 33 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4419b8134..1147c3236 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2398,6 +2398,7 @@ dependencies = [ "serde", "serde_json", "sqlx", + "test-case", "tokio", "tracing", ] diff --git a/crates/lib/docs_rs_repository_stats/Cargo.toml b/crates/lib/docs_rs_repository_stats/Cargo.toml index e3faf4fab..d8d68e247 100644 --- a/crates/lib/docs_rs_repository_stats/Cargo.toml +++ b/crates/lib/docs_rs_repository_stats/Cargo.toml @@ -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] diff --git a/crates/lib/docs_rs_repository_stats/src/github.rs b/crates/lib/docs_rs_repository_stats/src/github.rs index a10fb8a19..f0724b2cf 100644 --- a/crates/lib/docs_rs_repository_stats/src/github.rs +++ b/crates/lib/docs_rs_repository_stats/src/github.rs @@ -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(()) diff --git a/crates/lib/docs_rs_repository_stats/src/gitlab.rs b/crates/lib/docs_rs_repository_stats/src/gitlab.rs index 7eb52ed73..6b1336d8d 100644 --- a/crates/lib/docs_rs_repository_stats/src/gitlab.rs +++ b/crates/lib/docs_rs_repository_stats/src/gitlab.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(); diff --git a/crates/lib/docs_rs_repository_stats/src/retry.rs b/crates/lib/docs_rs_repository_stats/src/retry.rs index 144cb029c..1b2a3f28e 100644 --- a/crates/lib/docs_rs_repository_stats/src/retry.rs +++ b/crates/lib/docs_rs_repository_stats/src/retry.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) -> Option { - 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), } } }