[BUG] The curl retry path shares one jitter generator, redraws its deadline, and retries cancelled requests

[thc1006]

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);                                          // :641

Every 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);        // :607

No 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.

[thc1006]

Two corrections here, both mine.

First, this shouldn't be closed. The Fixes #4398 trailer 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_PREVIEW and never looking at the option that sets it. CMakeLists.txt:158 is option(WITH_OTLP_RETRY_PREVIEW "Whether to enable retry functionality" ON), api/CMakeLists.txt:93 warns that turning it off is deprecated, ci/do_ci.sh passes -DWITH_OTLP_RETRY_PREVIEW=ON at lines 182 and 407, and ext/src/http/client/curl/BUILD lists ENABLE_OTLP_RETRY_PREVIEW in defines with no select. 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 to NextRetryTime(), 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() at http_operation_curl.cc:594 still has no WasAborted(). Narrower than I made it sound above, though. It needs last_curl_result_ == CURLE_OK and 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]

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 on Response should, by what I wrote above, still see the request go out again. Three runs on 04a8ddd5, and the server saw one request every time:

PROBE requests=1 cancelled=1
PROBE requests=1 cancelled=1
PROBE requests=1 cancelled=1

Two reasons, and neither was in my description. The handler runs on the IO thread inside PerformCurlMessage, so its CancelSession() is queued before that pass reaches the retry step. And the pass runs doAbortSessions at http_client_curl.cc:538, doRemoveSessions at :544, doAddSessions at :550, then doRetrySessions at :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() at http_operation_curl.cc:594 does not look at cancellation.
• doAbortSessions() at http_client_curl.cc:759 calls FinishOperation() and nothing else. It never touches pending_to_retry_sessions_, which is only appended at :531 and erased inside doRetrySessions. 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 at http_operation_curl.cc:560 and HttpCurlEasyResource's move constructor nulls the source, so by the time doRetrySessions reaches that entry GetCurlEasyHandle() is already null. What actually runs is curl_multi_remove_handle(multi_handle_, nullptr) at :863 and curl_multi_add_handle(multi_handle_, nullptr) at :864. Measured against libcurl 8.14.1:

remove_handle(m, NULL) = 2  (Invalid easy handle)
add_handle(m, NULL)    = 2  (Invalid easy handle)
  CURLM_BAD_EASY_HANDLE = 2

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.