Repository navigation
[BUG] The curl retry path shares one jitter generator, redraws its deadline, and retries cancelled requests #4398
Description
Describe your environment main at f6e4818, Ubuntu 24.04, gcc 14, libcurl 8.14.1, CMake, -fsanitize=thread.
Three separate things in the retry path. The first is a data race and I have it under a sanitizer. The other two are from reading, and I say so.
1. The jitter generator is a shared mutable static
std::chrono::system_clock::time_point HttpOperation::NextRetryTime()
{
...
static std::random_device rd; // :621
static std::mt19937 gen(rd()); // :622
static std::uniform_real_distribution<float> dis(0.8f, 1.2f); // :623
...
backoff *= dis(gen); // :641Every HttpClient runs its own background thread, and dis(gen) advances the engine's state, so two clients retrying at once mutate one std::mt19937. Initialisation of a function local static is thread safe; using it is not.
NextRetryTime() is public and is not itself behind ENABLE_OTLP_RETRY_PREVIEW, so a test can call it directly and the static ships in every build:
TEST_F(BasicCurlHttpTests, RetryJitterRace)
{
...
HttpOperation a(Method::Get, "http://127.0.0.1:19000/", ssl, nullptr, headers, body);
HttpOperation b(Method::Get, "http://127.0.0.1:19000/", ssl, nullptr, headers, body);
std::thread t1([&a] { for (int i = 0; i < 200; ++i) { (void)a.NextRetryTime(); } });
std::thread t2([&b] { for (int i = 0; i < 200; ++i) { (void)b.NextRetryTime(); } });
t1.join();
t2.join();
}Three warnings per run, three runs out of three:
WARNING: ThreadSanitizer: data race
Read of size 8 by thread T3:
#0 std::mersenne_twister_engine<...>::operator()() /usr/include/c++/14/bits/random.tcc:458
#5 HttpOperation::NextRetryTime() ext/src/http/client/curl/http_operation_curl.cc:641
thread_local on the engine fixes it and keeps the jitter, which is the whole point of the draw.
2. The retry time is redrawn every time it is asked for
doRetrySessions() calls operation->NextRetryTime() on every pass of the IO loop, and NextRetryTime() recomputes backoff *= dis(gen) on every call. The deadline is therefore not a point in time, it is a fresh sample each time somebody asks. A session can be due on one pass and not due on the next.
That also undermines the ordering the queue assumes. pending_to_retry_sessions_ is a deque and the loop breaks at the first session that is not due yet, which is only correct if deadlines are ordered. They are not: per request retry policies differ, Retry-After can name any time, and now the jitter is redrawn under the comparison. #4186 already noted the sessions are no longer sorted by retry time.
Computing the deadline once, in PerformCurlMessage, and storing it would make the value stable. Whether the queue then needs anything cleverer than scanning it is a separate question, and for the number of retries an exporter has in flight, a scan is probably enough.
3. A cancelled session can still be retried
return is_retryable && (last_curl_result_ == CURLE_OK) &&
(retry_attempts_ < retry_policy_.max_attempts); // :607No WasAborted(). doRetrySessions() does not check it either, and it calls curl_multi_remove_handle and curl_multi_add_handle on GetCurlEasyHandle() without a null check or a look at whether the operation was cleaned. So a cancel that lands after doAbortSessions has run and before doRetrySessions does can leave a cancelled transfer queued, and re-arm it.
What is the expected behavior? Retry jitter is per thread or otherwise not shared, a retry deadline is decided once, and a cancelled request is not retried.
What is the actual behavior? The engine is shared across client threads, the deadline moves every time it is read, and cancellation is not part of the retry decision.
Additional context Item 1 stands alone and is small: it does not depend on anything else and it is the one I would send first if you want it. Items 2 and 3 are entangled with the queue, and item 3 overlaps the teardown ordering in #4391, so those are better after it.
All three are inside the retry preview in practice, since IsRetryable() returns false without ENABLE_OTLP_RETRY_PREVIEW and nothing enters the queue. The static in item 1 is compiled either way.
Found while reading the cancellation paths for #4390 and #4391.
Activity
thc1006 commentedon Aug 11, 2026
Two corrections here, both mine.
First, this shouldn't be closed. The
Fixes #4398trailer I put on #4399 closed all three items when it merged, and #4399 only covered item 1. I tried to reopen and GitHub won't let me, so item 2 is now #4403 with a measured reproduction. Item 3 I'm holding until #4391 lands, for the reason I gave in the description above.Second, the last paragraph of the description is wrong. I wrote that all three are "inside the retry preview in practice". They aren't. I got there by grepping for
ENABLE_OTLP_RETRY_PREVIEWand never looking at the option that sets it.CMakeLists.txt:158isoption(WITH_OTLP_RETRY_PREVIEW "Whether to enable retry functionality" ON),api/CMakeLists.txt:93warns that turning it off is deprecated,ci/do_ci.shpasses-DWITH_OTLP_RETRY_PREVIEW=ONat lines 182 and 407, andext/src/http/client/curl/BUILDlistsENABLE_OTLP_RETRY_PREVIEWindefineswith noselect. So the retry path is on in a default CMake build and in every Bazel build, and items 2 and 3 are not opt-in. Sorry for underselling that.Item 2, re-checked at
768d04de: one attempt, eight calls toNextRetryTime(), eight different deadlines, spread of 322, 335 and 350 ms across three runs. The mechanism, the test and a fix shape are on #4403.Item 3, re-checked at
768d04de:IsRetryable()athttp_operation_curl.cc:594still has noWasAborted(). Narrower than I made it sound above, though. It needslast_curl_result_ == CURLE_OKand a 429, 502, 503 or 504, so the case is a cancel that lands after the server has already answered with a retryable status, not any cancel at all. I'll write that one up properly rather than leave the vaguer version standing.thc1006 commentedon Aug 11, 2026
Writing item 3 up properly, as promised. Doing that turned out to mean correcting it downwards rather than filing it as stated, so it stays here rather than becoming its own issue.
I tried to reproduce it first.
/retry/answers 429, so a session with a real retry policy is retryable, and a handler that cancels onResponseshould, by what I wrote above, still see the request go out again. Three runs on04a8ddd5, and the server saw one request every time:Two reasons, and neither was in my description. The handler runs on the IO thread inside
PerformCurlMessage, so itsCancelSession()is queued before that pass reaches the retry step. And the pass runsdoAbortSessionsathttp_client_curl.cc:538,doRemoveSessionsat:544,doAddSessionsat:550, thendoRetrySessionsat:556, so an abort is always cleaned up and its handle freed before any retry is considered.What is still true, checked at
04a8ddd5:IsRetryable()athttp_operation_curl.cc:594does not look at cancellation.doAbortSessions()athttp_client_curl.cc:759callsFinishOperation()and nothing else. It never touchespending_to_retry_sessions_, which is only appended at:531and erased insidedoRetrySessions. So a session cancelled while it waits out a backoff stays in that queue.What is not true is the part I put most weight on. "Re-arm it" cannot happen:
Cleanup()moves the resource out athttp_operation_curl.cc:560andHttpCurlEasyResource's move constructor nulls the source, so by the timedoRetrySessionsreaches that entryGetCurlEasyHandle()is already null. What actually runs iscurl_multi_remove_handle(multi_handle_, nullptr)at:863andcurl_multi_add_handle(multi_handle_, nullptr)at:864. Measured against libcurl 8.14.1:Both returns are dropped, and the entry is erased. No request goes out.
So the residue is smaller than one issue's worth. A cancelled session lingers in the retry queue until its own backoff expires, and since the loop
breaks at the first entry that is not due yet, one at the head holds up the entries behind it for that long. Then two error codes get discarded.That leaves it in two places that already exist rather than a new one. The discarded return codes are the same class as #4404. The head of line behaviour belongs with the queue question on #4403, since a stable deadline is what would let that queue be reasoned about at all. I would rather say that than open a third issue on the strength of a claim I have just walked back.