[BUG] A send during WaitBackgroundThreadExit starts a second background thread, and teardown races it #4665

Describe your environment

• WaitBackgroundThreadExit() came in with [SDK] Do not frequently create and destroy http client threads #3198 and first shipped in v1.19.0. The re-swap loop it is missing has been in the destructor since Merge all commits of async-changes into main #1413 and v1.5.0. main at 489cd823 still has both shapes.
• Ubuntu 24.04, gcc 14, libcurl 8.14.1, CMake, built with OTELCPP_WITH_THREAD_INSTRUMENTATION_PREVIEW=ON. That flag is what makes the ordering below deterministic instead of timed; the code it exercises is not behind it and is not platform specific.

Steps to reproduce

1. Give an ext::http::client::curl::HttpClient a ThreadInstrumentation whose AfterWait() blocks until the test releases it. AfterWait is called at http_client_curl.cc:525 and holds no lock. OnEnd will not do for this: it runs at :648, inside the guard taken at :612, which is the mutex the spawn needs, so blocking there blocks the spawn instead of letting it through.
2. Call MaybeSpawnBackgroundThread() and wait until the thread is parked in AfterWait.
3. From another thread, call WaitBackgroundThreadExit(). It swaps background_thread_ out at :723, releases the lock at :724, and blocks in the join at :729 because the thread it took is parked.
4. Send a request. Session::SendRequest calls MaybeSpawnBackgroundThread() at :258, it finds a null member, and a second thread starts. Point the request at an address that does not answer, https://192.0.2.0:19000/get/ as SendGetRequestAsyncTimeout already does, so the new thread has a session to service rather than nothing to do.
5. Release the parked thread, join the caller of WaitBackgroundThreadExit(), and count the background threads that have started and not ended.

What is the expected behavior?

Once WaitBackgroundThreadExit() returns, no background thread of that client is running. The destructor already behaves that way. Lines :322 to :342 swap and join in a loop until a swap comes back empty, so a thread that appears during teardown is collected on the next turn.

What is the actual behavior?

The wait joins the thread it swapped out and returns whether or not another has appeared:

719:  is_shutdown_.store(true, ...);
722:    std::lock_guard<std::mutex> lock_guard{background_thread_m_};
723:    background_thread.swap(background_thread_);    <- member is null from here
724:  }                                               <- lock released
729:    background_thread->join();                    <- joins only what it swapped out
731:  is_shutdown_.store(false, ...);

MaybeSpawnBackgroundThread() takes the same mutex at :471 and then tests background_thread_ at :472 and nothing else. From :724 to :729 that member is null, so the test passes and a thread starts. is_shutdown_ is true for the whole span and the spawn never reads it.

Nor does the new thread simply retire again. With a session queued, doAddSessions() at :629 sets still_running and it stays in the loop. Five runs out of five, with the ordering forced by the gate rather than by timing: two threads started, and one of them was still in its loop at the moment the wait returned.

Teardown then races it. Finishing the session, calling FinishAllSessions() and then destroying the client, which is what a caller would do next, gives ThreadSanitizer this, three runs out of three:

SUMMARY: ThreadSanitizer: data race in curl_multi_cleanup
  Write of size 8 by main thread (mutexes: write M0):
    curl_multi_cleanup                       <- from ~HttpClient
  Previous read of size 8 by thread T4:
    libcurl.so.4
  Location is file descriptor 5 created by main thread
  Thread T4 created by main thread at:
    Session::SendRequest(...) http_client_curl.cc:258

T4 is the thread the send started. All 16 reports in that run have the main thread on one side and T4 on the other, and 13 of them name ~HttpClient.

Additional context

The race needs both halves, so here is the grid rather than the one cell that fails. Each row is the same TSan build, run on its own:

Two existing cases already destroy a client while a background thread runs, and they are clean. Neither the window alone nor the work alone produces anything.

What I have not instrumented is how the second thread gets past the destructor's join loop. My reading is the self-detach at :654: the thread retires, finds its own handle in background_thread_, detaches and clears it, and the loop at :322 then swaps out an empty pointer and breaks. That is a reading of the code, not a measurement, and I would rather hand it over as one.

Nothing in this repository calls WaitBackgroundThreadExit() outside ext/test/http/curl_http_test.cc, where it is called six times, so no shipped code path reaches this today. It is public API.

This is not #4402. That one parks the single IO thread from inside a handler. This is the opposite: a second IO thread where the design allows one.

None of #4395, #4405 or #4406 covers it. They change the loop body and the retry path, and leave the swap at :723 and the test at :472 as they are.

#4664 is the other open report on this loop, filed the same day: the CURLMcode from curl_multi_poll at :513 is stored and never read. Different defect, same function.

I would like to take it. The destructor's loop is the shape that already works here; the other option is for MaybeSpawnBackgroundThread() to refuse while is_shutdown_ is set. The second changes what a send does during shutdown, so it is worth a word from you before I write either one.

_{Tip: React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.}