From 642a2651ff241fd2d8913a26e49c1320bc9e8d92 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 23 Sep 2026 08:45:50 -0700 Subject: [PATCH 1/3] [BUG] Do not queue a curl session closed by the Retry-After cap --- .../http/client/curl/http_operation_curl.h | 3 +- ext/src/http/client/curl/http_client_curl.cc | 4 +- .../http/client/curl/http_operation_curl.cc | 5 +- ext/test/http/curl_http_test.cc | 60 +++++++++++++++++++ 4 files changed, 67 insertions(+), 5 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index ce62d9f1e7..6987ea5ff0 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -298,8 +298,9 @@ class HttpOperation * be called when got a CURLMSG_DONE. * * @param code CURLcode + * @return true if the request was re-armed for a retry, false if the operation was cleaned up */ - void PerformCurlMessage(CURLcode code); + bool PerformCurlMessage(CURLcode code); inline CURL *GetCurlEasyHandle() noexcept { return curl_resource_.easy_handle; } diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index d900c74b1b..92432cc79f 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -534,9 +534,7 @@ bool HttpClient::MaybeSpawnBackgroundThread() { // Session can not be destroyed when calling PerformCurlMessage auto hold_session = session->shared_from_this(); - operation->PerformCurlMessage(result); - - if (operation->IsRetryable()) + if (operation->PerformCurlMessage(result)) { self->pending_to_retry_sessions_.push_back(hold_session); } diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 8d399d5f2a..a46bd3f3d2 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1561,7 +1561,7 @@ void HttpOperation::Abort() } } -void HttpOperation::PerformCurlMessage(CURLcode code) +bool HttpOperation::PerformCurlMessage(CURLcode code) { ++retry_attempts_; last_attempt_time_ = std::chrono::system_clock::now(); @@ -1667,7 +1667,10 @@ void HttpOperation::PerformCurlMessage(CURLcode code) { // Cleanup and unbind easy handle from multi handle, and finish callback Cleanup(); + return false; } + + return true; } } // namespace curl diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 0d7d09bd7f..32287dfb1b 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -262,6 +262,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe server_.addHandler("/get/", *this); server_.addHandler("/post/", *this); server_.addHandler("/retry/", *this); + server_.addHandler("/retry-after/", *this); server_.addHandler("/close/", *this); server_.start(); is_running_ = true; @@ -302,6 +303,14 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe response.headers["Content-Type"] = "text/plain"; response_status = 429; } + else if (request.uri == "/retry-after/") + { + std::unique_lock lk1(mtx_requests); + received_requests_.push_back(request); + response.headers["Content-Type"] = "text/plain"; + response.headers["Retry-After"] = "30"; + response_status = 429; + } else if (request.uri == "/close/") { // -1 is the documented way for a handler to ask the server to terminate the @@ -638,6 +647,57 @@ TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry) ASSERT_EQ(CURLE_OK, operation.Send()); ASSERT_FALSE(operation.IsRetryable()); } + +// A Retry-After beyond max_backoff closes the session. The IO loop used to queue it anyway, where +// it held back later retries and the background thread until the server's time. +TEST_F(BasicCurlHttpTests, RetryAfterBeyondMaxBackoffIsNotQueued) +{ + received_requests_.clear(); + curl::HttpClient http_client; + const http_client::RetryPolicy retry_policy = {2, std::chrono::duration{0.1f}, + std::chrono::duration{1.0f}, 1.0f}; + + auto capped_session = http_client.CreateSession("http://127.0.0.1:19000"); + auto capped_request = capped_session->CreateRequest(); + capped_request->SetMethod(http_client::Method::Post); + capped_request->SetUri("retry-after/"); + capped_request->SetRetryPolicy(retry_policy); + auto capped_handler = std::make_shared(); + capped_session->SendRequest(capped_handler); + capped_session->FinishSession(); + ASSERT_TRUE(capped_handler->got_response_.load(std::memory_order_acquire)); + + auto session = http_client.CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetMethod(http_client::Method::Post); + request->SetUri("retry/"); + request->SetRetryPolicy(retry_policy); + auto handler = std::make_shared(); + auto started_at = std::chrono::steady_clock::now(); + session->SendRequest(handler); + session->FinishSession(); + const auto retried_in = std::chrono::steady_clock::now() - started_at; + ASSERT_TRUE(handler->got_response_.load(std::memory_order_acquire)); + + started_at = std::chrono::steady_clock::now(); + http_client.WaitBackgroundThreadExit(); + const auto joined_in = std::chrono::steady_clock::now() - started_at; + + // The server asks for 30 s; the policy backs off for about 0.1 s. + EXPECT_TRUE(retried_in < std::chrono::seconds{10}) + << "retry ms: " << std::chrono::duration_cast(retried_in).count(); + EXPECT_TRUE(joined_in < std::chrono::seconds{10}) + << "join ms: " << std::chrono::duration_cast(joined_in).count(); + + std::unique_lock lock_requests(mtx_requests); + const auto hits = [this](const char *uri) { + return std::count_if( + received_requests_.begin(), received_requests_.end(), + [uri](const HTTP_SERVER_NS::HttpRequest &received) { return received.uri == uri; }); + }; + EXPECT_EQ(1, hits("/retry-after/")); + EXPECT_EQ(2, hits("/retry/")); +} #endif // ENABLE_OTLP_RETRY_PREVIEW // A cancel that arrives once the server has answered used to deliver Cancelled and the response, From 5159e3b321528f1395ed6b6f156493cad880f1e5 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 23 Sep 2026 08:58:16 -0700 Subject: [PATCH 2/3] Add changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e203290157..a322efd636 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,9 @@ Increment the: to compile standalone on newer standard library implementations. [#4574](https://github.com/open-telemetry/opentelemetry-cpp/pull/4574) +* [BUG] Do not queue a curl session closed by the Retry-After cap + [#4632](https://github.com/open-telemetry/opentelemetry-cpp/pull/4632) + ## [1.29.0] 2026-09-13 * [RELEASE] Bump main branch to 1.29.0-dev (#4259) From 926e2492f249e83bd4cd1f48c2a6d30a3641e0f2 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 23 Sep 2026 19:23:05 -0700 Subject: [PATCH 3/3] [TEST] Assert the Retry-After cap does not delay curl client shutdown --- ext/test/http/curl_http_test.cc | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 32287dfb1b..9e93294b75 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -698,6 +698,39 @@ TEST_F(BasicCurlHttpTests, RetryAfterBeyondMaxBackoffIsNotQueued) EXPECT_EQ(1, hits("/retry-after/")); EXPECT_EQ(2, hits("/retry/")); } + +// The shutdown half of #4631: the closed session used to hold the join until the server's time. +TEST_F(BasicCurlHttpTests, RetryAfterBeyondMaxBackoffDoesNotDelayShutdown) +{ + received_requests_.clear(); + curl::HttpClient http_client; + const http_client::RetryPolicy retry_policy = {2, std::chrono::duration{0.1f}, + std::chrono::duration{1.0f}, 1.0f}; + + auto session = http_client.CreateSession("http://127.0.0.1:19000"); + auto request = session->CreateRequest(); + request->SetMethod(http_client::Method::Post); + request->SetUri("retry-after/"); + request->SetRetryPolicy(retry_policy); + auto handler = std::make_shared(); + session->SendRequest(handler); + session->FinishSession(); + ASSERT_TRUE(handler->got_response_.load(std::memory_order_acquire)); + + const auto started_at = std::chrono::steady_clock::now(); + http_client.WaitBackgroundThreadExit(); + const auto joined_in = std::chrono::steady_clock::now() - started_at; + + // The server asks for 30 s. + EXPECT_TRUE(joined_in < std::chrono::seconds{10}) + << "join ms: " << std::chrono::duration_cast(joined_in).count(); + + std::unique_lock lock_requests(mtx_requests); + EXPECT_EQ(1, std::count_if(received_requests_.begin(), received_requests_.end(), + [](const HTTP_SERVER_NS::HttpRequest &received) { + return received.uri == "/retry-after/"; + })); +} #endif // ENABLE_OTLP_RETRY_PREVIEW // A cancel that arrives once the server has answered used to deliver Cancelled and the response,