From 051761dd61ba2833d2725442eec6f42d82a4a6f3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Fri, 11 Sep 2026 17:13:40 -0400 Subject: [PATCH] [upstream handoff] fix: apply a request timeout to every forge HTTP client This branch is based on upstream v0.5.4 (0c0341846), not on this fork's main. Matt pulls it and pushes it to the upstream Codeberg repo (codeberg.org/abrenneke/jj-vine). The two sections below are the text to open the upstream PR with; they are the deliverable, not this fork PR's own description. ## Upstream PR title fix: apply a request timeout to every forge HTTP client ## Upstream PR description `reqwest` applies no request timeout by default. A forge that accepts the TCP connection but never sends a response leaves the client blocked in the socket read indefinitely, with no output and no way out but an external kill. The GitHub, GitLab, Forgejo, and Azure DevOps clients are each built from a bare `reqwest::Client::builder()`, so none of them has a timeout. This routes all four constructors through a shared `http_client_builder()` that applies a 30-second timeout, so a client cannot be built without one. The forge constructors still add their own TLS and CA options on top before calling `.build()`. Adds a test that points a client at a listener which accepts and never responds: with the timeout the request errors within a few seconds; a control using the old bare builder is still pending when a short outer deadline fires. Co-authored-by: Matt Wilkinson --- src/forge.rs | 110 +++++++++++++++++++++++++++++++++++++++++++ src/forge/azure.rs | 2 +- src/forge/forgejo.rs | 2 +- src/forge/github.rs | 2 +- src/forge/gitlab.rs | 2 +- 5 files changed, 114 insertions(+), 4 deletions(-) diff --git a/src/forge.rs b/src/forge.rs index 3d651e9..c7f4b5a 100644 --- a/src/forge.rs +++ b/src/forge.rs @@ -4,6 +4,7 @@ pub mod github; pub mod gitlab; pub mod test; +use core::time::Duration; use std::borrow::Cow; use bon::Builder; @@ -18,6 +19,28 @@ use crate::{ utils::ResultWithWarnings, }; +/// Default timeout applied to every forge HTTP client. +/// +/// `reqwest` has no request timeout by default, so a connection that stalls +/// after connecting — a forge that accepts the socket but never responds — +/// hangs the whole process with no output and no error. Every forge client is +/// built through [`http_client_builder`] so the timeout is applied uniformly. +pub(crate) const HTTP_TIMEOUT: Duration = Duration::from_secs(30); + +/// A `reqwest` client builder pre-configured with [`HTTP_TIMEOUT`]. +/// +/// Forge constructors add their TLS/CA options on top of this and then +/// `.build()` it, so no client can be created without the timeout. +pub(crate) fn http_client_builder() -> reqwest::ClientBuilder { + http_client_builder_with_timeout(HTTP_TIMEOUT) +} + +/// A `reqwest` client builder with an explicit request timeout. Split out so +/// tests can exercise the timeout path with a short duration. +pub(crate) fn http_client_builder_with_timeout(timeout: Duration) -> reqwest::ClientBuilder { + reqwest::Client::builder().timeout(timeout) +} + pub trait UserLike: core::fmt::Debug { fn id(&self) -> Option>; @@ -1249,3 +1272,90 @@ impl ForgeImpl { } } } + +#[cfg(test)] +mod http_timeout_tests { + use core::time::Duration; + use std::net::TcpListener; + + use super::{HTTP_TIMEOUT, http_client_builder, http_client_builder_with_timeout}; + + /// Bind a listener that accepts connections and never responds, then leak + /// it for the rest of the test process. This is the RIG-3585 failure: the + /// client connects, sends its request, and blocks in `recvfrom` forever. + /// + /// The listener is intentionally `forget`-leaked rather than run on a + /// spawned thread: the OS accepts connections into the socket's backlog + /// without any user-space `accept()`, so a bound-but-unaccepted socket is + /// a black hole on its own. That keeps the helper free of a background + /// thread and file descriptor that would outlive each test. + fn black_hole() -> String { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind black-hole listener"); + let addr = listener.local_addr().expect("read local addr"); + // Keep the port bound (and the backlog accepting) for the whole test + // process without holding a `JoinHandle` we would have to reap. + core::mem::forget(listener); + format!("http://{addr}/") + } + + #[tokio::test] + async fn client_times_out_instead_of_hanging() { + let url = black_hole(); + + // A client with a short timeout must return a timeout error rather than + // blocking on the never-answering socket. The outer `tokio` deadline is + // a guard, not the assertion: if the client's own timeout regresses, + // `send()` never returns and the outer deadline makes the test panic + // fast instead of hanging the whole CI job (the CI `test` job sets no + // `timeout-minutes`). + let client = http_client_builder_with_timeout(Duration::from_millis(300)) + .build() + .expect("build client"); + + let raced = tokio::time::timeout(Duration::from_secs(5), client.get(&url).send()).await; + let result = raced + .expect("client hung past its own 300ms timeout — the request timeout was not applied"); + let err = result.expect_err("request to a black hole must error, not succeed"); + assert!( + err.is_timeout(), + "the error must be a timeout, got: {err:?}" + ); + } + + #[tokio::test] + async fn a_timeoutless_client_would_hang() { + // Demonstrates the bug this fix prevents: the pre-fix inline builder + // (`reqwest::Client::builder()` with no `.timeout()`) never returns + // against a black hole. We prove that by racing it against a short + // outer deadline; the request is still pending when the deadline fires. + let url = black_hole(); + + let timeoutless = reqwest::Client::builder() + .build() + .expect("build timeoutless client"); + + let raced = + tokio::time::timeout(Duration::from_millis(500), timeoutless.get(&url).send()).await; + + assert!( + raced.is_err(), + "without a timeout the request must still be pending at the deadline" + ); + } + + #[test] + fn production_client_builder_applies_the_timeout() { + // Guards the actual claim of this fix: the helper every forge + // constructor uses (`http_client_builder`) wires `HTTP_TIMEOUT` into the + // client. `reqwest::Client`'s `Debug` renders the configured deadline, + // so this needs no socket. Without the `.timeout()` this reads + // `Client { accepts: Accepts, .. }` with no `TotalTimeout`, so the + // assertion is genuinely red-green on the production path. + assert_eq!(HTTP_TIMEOUT, Duration::from_secs(30)); + let rendered = format!("{:?}", http_client_builder().build().expect("build client")); + assert!( + rendered.contains("TotalTimeout: 30s"), + "http_client_builder() must apply the 30s timeout, got: {rendered}" + ); + } +} diff --git a/src/forge/azure.rs b/src/forge/azure.rs index c9c66ea..ac2be0b 100644 --- a/src/forge/azure.rs +++ b/src/forge/azure.rs @@ -152,7 +152,7 @@ impl AzureDevOpsForge { ca_bundle: Option>, accept_non_compliant_certs: bool, ) -> Result { - let mut client_builder = reqwest::Client::builder(); + let mut client_builder = crate::forge::http_client_builder(); if accept_non_compliant_certs { client_builder = client_builder.tls_danger_accept_invalid_certs(true); diff --git a/src/forge/forgejo.rs b/src/forge/forgejo.rs index 1282e67..d14b89e 100644 --- a/src/forge/forgejo.rs +++ b/src/forge/forgejo.rs @@ -308,7 +308,7 @@ impl ForgejoForge { accept_non_compliant_certs: bool, wip_prefix: String, ) -> Result { - let mut client_builder = reqwest::Client::builder(); + let mut client_builder = crate::forge::http_client_builder(); if accept_non_compliant_certs { client_builder = client_builder.tls_danger_accept_invalid_certs(true); diff --git a/src/forge/github.rs b/src/forge/github.rs index d815cb8..0d3d6b2 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -374,7 +374,7 @@ impl GitHubForge { ca_bundle: Option>, accept_non_compliant_certs: bool, ) -> Result { - let mut client_builder = reqwest::Client::builder(); + let mut client_builder = crate::forge::http_client_builder(); if accept_non_compliant_certs { client_builder = client_builder.tls_danger_accept_invalid_certs(true); diff --git a/src/forge/gitlab.rs b/src/forge/gitlab.rs index 6ad11fe..bf62a70 100644 --- a/src/forge/gitlab.rs +++ b/src/forge/gitlab.rs @@ -141,7 +141,7 @@ impl GitLabForge { accept_non_compliant_certs: bool, create_merge_request_dependencies: bool, ) -> Result { - let mut client_builder = reqwest::Client::builder(); + let mut client_builder = crate::forge::http_client_builder(); // Accept non-compliant certificates if configured if accept_non_compliant_certs {