[BUG] The curl client writes HttpOperation::session_state_ from two threads without synchronization #4408

Line numbers, re-resolved 19 October 2026. Everything quoted below is at f1e1c9cb, which the environment section names. Against main at cdd17eb1 the file has moved, so here is where each cited line is now.

cited	what it is	on cdd17eb1
http_operation_curl.cc:415	the session_state_ = type; store in DispatchEvent	432, with DispatchEvent at 424 and the OnEvent call at 429
http_operation_curl.cc:1455	HttpOperation::SendAsync	1459
http_operation_curl.cc:536	the Cancelled dispatch in Cleanup()	553
http_client_curl.cc:209	the completion lambda passed to SendAsync	234
http_client_curl.cc:269	Session::FinishOperation	290
http_client_curl.cc:777	HttpClient::doAbortSessions	784

One attribution to correct. The body says #4395 removes the write to async_data_->session racing the read of it in Cleanup(). That one was closed on main by #4392, which made the member std::atomic: it is stored at 1497 and exchanged at 566. The race this issue reports, the plain session_state_ = type; after the handler has run, is still live at 432.

Describe your environment main at f1e1c9c, Linux, gcc 14.2, libcurl 8.14.1, CMake build with -fsanitize=thread. The race is in the source rather than in a build option, so the Bazel --config=tsan toolchain sees the same thing.

Steps to reproduce

HttpOperation::DispatchEvent stores the new state after the handler has returned, in http_operation_curl.cc:

void HttpOperation::DispatchEvent(SessionState type, const std::string &reason)
{
  if (event_handle_ != nullptr)
  {
    event_handle_->OnEvent(type, reason);
  }

  session_state_ = type;
}

session_state_ is a plain SessionState member (http_operation_curl.h, and SessionState is a std::uint8_t enum). Two threads reach that store for one operation whenever a handler cancels from an early event on a client that is already polling:

1. Send a request on a fresh client and let it finish. MaybeSpawnBackgroundThread() runs after SendAsync returns, so this first request is what puts an IO thread there at all.
2. Create a second session and call CancelSession() from its Connecting event, without leaving the handler yet.
3. The cancel wakes the IO thread, which runs doAbortSessions (http_client_curl.cc) into Session::FinishOperation into HttpOperation::Cleanup. Cleanup finds a non terminal state and dispatches a Cancelled of its own, which stores session_state_ on the IO thread.
4. The handler returns and DispatchEvent stores session_state_ = Connecting on the caller thread.

I held the handler open for 400 ms so the overlap is wide enough to be deterministic rather than something I got lucky on. Three TSAN runs out of three report it, and five out of five reach the overlap on the behavioral build.

What is the expected behavior?

Either one thread owns the store, or the store is synchronized. And a state the operation has already left should not replace a terminal one.

What is the actual behavior?

TSAN reports a data race on the store. Trimmed to the two stacks:

WARNING: ThreadSanitizer: data race
  Write of size 1 by main thread:
    #0 HttpOperation::DispatchEvent(...) http_operation_curl.cc:415
    #1 HttpOperation::SendAsync(...) http_operation_curl.cc:1455
    #2 Session::SendRequest(...) http_client_curl.cc:209

  Previous write of size 1 by thread T2:
    #0 HttpOperation::DispatchEvent(...) http_operation_curl.cc:415
    #1 HttpOperation::Cleanup() http_operation_curl.cc:536
    #2 Session::FinishOperation() http_client_curl.cc:269
    #3 HttpClient::doAbortSessions() http_client_curl.cc:777

Past the undefined behavior, the later store wins, so GetSessionState() ends at Connecting for an operation that was cancelled. Two places read that member and act on it: Cleanup decides from it whether to dispatch at all, and the completion callback in Session::SendRequest compares it against Response to decide whether to deliver a response.

Additional context

Nothing in ext/test/http/curl_http_test.cc reaches this today, which is why Bazel tsan config is green. Every case that cancels from an early event uses a freshly created client, and there the IO thread does not exist while the event runs, so there is only one writer. That is measured rather than assumed: the same probe without the first request reports one thread inside the handler, and with it, two.

A warning for whoever writes the regression test. My first attempt counted concurrent handler entries with acq_rel atomics and TSAN went quiet. The IO thread's release RMW and the caller's later RMW on the same atomic synchronize with each other, which orders the two stores and hides the race the counters exist to catch. They have to be relaxed. I checked that with a deliberate unsynchronized counter on the same line as a control: the acq_rel build reported neither, the relaxed build reported both.

On main this scenario reports two races and then hangs. #4395 removes the hang and the other one, a write to async_data_->session in SendAsync racing the read of it in Cleanup, and leaves this one. So this is not something #4395 introduces, and it is not something #4395 fixes either.

Two shapes for a fix, and the choice does not look like mine to make:

• Make session_state_ a std::atomic. Removes the undefined behavior, leaves the later writer winning, so a cancelled operation can still report Connecting. It also changes the layout of a type in an installed header.
• Store before dispatching and refuse to leave a terminal state. That fixes the reported value too, but it changes what a handler sees when it asks GetSessionState() from inside its own event, which today is the state before the one it is being told about.

Scheduling note, same as on #4360. #4392 and #4395 are open against these two files. Tell me which shape you want and I will fit it around whatever has landed rather than sending a change that collides with them. It also holds up the regression test for the polling client half of #4390, because that test turns Bazel tsan config red on this race.