[BUG] Fix a data race on the curl operation's last result code

[thc1006]

Fixes #4614.

The interleaving

On the IO thread, draining one curl message:

1. PerformCurlMessage() stores the transfer result into last_curl_result_, and on an attempt that will not be retried it reaches Cleanup(), which calls result_promise.set_value(last_curl_result_).
2. That releases the thread blocked in Finish(), which stored the same value back into last_curl_result_.
3. The IO thread returns from PerformCurlMessage() and calls operation->IsRetryable(), which reads last_curl_result_.

Steps 2 and 3 are unordered, which is the pair in the report: a write of size 4 under cleanupGCSessions() against a read of size 4 in the background thread. CURLcode is 4 bytes here and response_code_, the other member IsRetryable() reads, is a long at 8, which is how the sizes identify it. ~HttpOperation() has the same wait then store, so it is the second write site.

The change

Drop both stores.

The waiter has nothing to write. Cleanup() sets the promise from last_curl_result_, and the future orders that write ahead of whoever is waiting, so by the time get() returns the member already holds the value the waiter was about to assign to it. Dropping the store leaves the member with one writer and removes the race rather than making it benign.

get() is still called, so the future is consumed exactly as before and nothing else about the wait changes. The discard is written out with static_cast<void> rather than left implicit.

Why no later attempt can produce a newer code

@shrimech asked whether the operation can be redriven between Cleanup() setting the promise and the store running, which would have made the store overwrite a newer code with a stale one. It cannot, for three independent reasons, and the change removes the question anyway.

1. A retryable attempt never reaches Cleanup(). At the end of PerformCurlMessage() it is called only from if (!is_retryable || retry_after_exceeds_max_delay). A retry takes the other branch, which rewinds and dispatches Connecting, and returns. So the promise is only ever set on the attempt that will not be retried.
2. Cleanup() clears the operation's back pointer. It sets CURLOPT_PRIVATE to null and hands curl_resource_ to ScheduleRemoveSession(), which queues the curl_multi_remove_handle() for the IO thread rather than performing it there and then. The message loop resolves the operation through CURLINFO_PRIVATE, so once that is null it skips the message without calling PerformCurlMessage(), whether or not the handle has left the multi yet.
3. Cleanup() runs once. is_cleaned_.exchange(true) returns early on re-entry, and is_promise_running.exchange(false) independently caps set_value at one, so neither Session::FinishOperation() nor the destructor can produce a second one.

doRetrySessions() also only re-adds the existing easy handle with curl_multi_add_handle(); it does not go back through SendAsync(), so it never resets is_cleaned_.

That chain spans three functions in two files and nothing states it, which is a good argument for not depending on it. Without the store there is nothing to go stale: if a future change ever did let a later attempt run, the member would simply hold the newer code.

Evidence

main at 1525d6a5, gcc 14.2, Bazel:

bazel build --config=tsan --//api:with_cxx_stdlib=none //exporters/otlp:otlp_http_exporter_test
./bazel-bin/exporters/otlp/otlp_http_exporter_test --gtest_filter='*RetryIntegration*'

tree	runs	races
main	6	6, all the same Finish() / IsRetryable() pair
with this change	6	0

All six clean runs used TSAN_OPTIONS=report_atomic_races=1. All 40 cases pass, //ext/test/http/... passes, and the plain non-sanitised build passes.

That configuration is where it reproduces every time. Under the flags the bazel.tsan job actually uses I did not hit it in six runs on unpatched main, and the patched tree is clean over three, which fits this being reported as an intermittent CI failure rather than a reliable one.

What this does not fix

Only the reported access. The IO thread still consults an operation after another thread has been released to finish or destroy it; the shared_from_this() hold is what keeps that safe today. GetLastResultCode() is also still a plain read of a member the IO thread writes, which is fine while it has no callers, but it is not a thread-safe accessor.

An earlier revision of this pull request made the member std::atomic<CURLcode> instead. That also passes, but it keeps the cross thread write and only makes it defined, so this version is the smaller and more direct one. Happy to go back to it if you would rather have the type level guarantee for GetLastResultCode().

On testing

There is no new unit test, because I could not write one that fails deterministically without a sanitiser. The regression test is the existing OtlpHttpExporterRetryIntegrationTests under --config=tsan, which is what caught it. The honest gap is that the bazel.tsan job does not use the configuration where it fires every time, so a regression there would surface the way this one did, intermittently.

[shrimech]

can you confirm the operation can't be redriven for a retry (PerformCurlMessage() storing a newer code) between Cleanup() setting the promise and this store running? If it can the store would overwrite the newer value with a stale one

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.53%. Comparing base (4679325) to head (8f17275).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ext/src/http/client/curl/http_operation_curl.cc	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4618      +/-   ##
==========================================
+ Coverage   86.53%   86.53%   +0.01%     
==========================================
  Files         525      525              
  Lines       20482    20480       -2     
==========================================
- Hits        17722    17721       -1     
+ Misses       2760     2759       -1     

Files with missing lines	Coverage Δ
ext/src/http/client/curl/http_operation_curl.cc	61.26% <50.00%> (+0.04%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thc1006]

Good question, and it pushed the change in a better direction. The short answer is that it cannot happen today, but the store was the wrong thing to defend, so I removed it instead.

Why a newer code cannot exist between Cleanup() setting the promise and the store:

1. A retryable attempt never reaches Cleanup(). At the end of PerformCurlMessage() it is called only from if (!is_retryable || retry_after_exceeds_max_delay); a retry takes the other branch, rewinds, dispatches Connecting and returns. The promise is only set on the attempt that will not be retried.
2. Cleanup() clears the operation's back pointer. It sets CURLOPT_PRIVATE to null and hands curl_resource_ to ScheduleRemoveSession(), which queues the curl_multi_remove_handle() for the IO thread rather than doing it there. The message loop resolves the operation through CURLINFO_PRIVATE, so after that it skips the message without calling PerformCurlMessage(), whether or not the handle has left the multi yet.
3. Cleanup() runs once, guarded by is_cleaned_.exchange(true), and is_promise_running.exchange(false) caps set_value at one independently.

doRetrySessions() only re-adds the existing easy handle with curl_multi_add_handle(), so it never goes back through SendAsync() or resets is_cleaned_.

That is three functions across two files holding one unwritten invariant, which is a thin thing to rely on. So rather than argue the store is safe, the branch now drops it: Cleanup() sets the promise from last_curl_result_ and the future orders that write ahead of the waiter, so the member already holds the value the waiter was assigning. One writer, no race, and if a later attempt ever could run, the member would hold the newer code rather than a stale one.

The diff is down to four lines in http_operation_curl.cc. Same measurement as before: 6 of 6 runs race on main, 0 of 6 with the change, under --config=tsan --//api:with_cxx_stdlib=none with report_atomic_races=1.

Edited: I had written that Cleanup() detaches the handle. It schedules the removal; the curl_multi_remove_handle() runs later on the IO thread. The null CURLOPT_PRIVATE is what makes the loop skip the message, and that does not depend on the removal having happened.

[shrimech]

no issues from my side. Fix is correct and the removal of the redundant write-backs is a solid improvement over the original approach.

[thc1006]

Marking this ready. @shrimech's question turned out to be the useful one: the first version made last_curl_result_ atomic, and after chasing whether a later attempt could overwrite it, the better answer was that the two stores are writing back a value the IO thread has already published. So the change is four lines in http_operation_curl.cc and no header or type change at all.

@marcalff, on your question in #4614, it is a new race rather than #4408.

[owent]

LGTM, thanks.

[marcalff]

LGTM,

Great work, not only with the fix but more importantly on the analysis.