[BUG] Scope the session lock to the snapshot in resetMultiHandle

[thc1006]

Fixes #4389.

resetMultiHandle() took sessions_m_ for the whole function and then called two things that take it again on the same thread: CancelSession(), which reaches CleanupSession(), and doRemoveSessions(). sessions_m_ is a plain std::mutex, so the IO thread stopped there and never came back. That is the path that recovers from a curl_multi_perform error, so the failure mode is a background thread that quietly stops existing while everything queued behind it waits.

The lock only ever guarded the snapshot. It is scoped to that now, which is the whole production change:

  std::list<std::shared_ptr<Session>> sessions;
  {
    std::lock_guard<std::mutex> session_lock_guard{sessions_m_};
    std::lock_guard<std::recursive_mutex> session_id_lock_guard{session_ids_m_};
    ...
  }

  for (auto &session : sessions)
  {
    session->CancelSession();
    session->FinishOperation();
  }

  doRemoveSessions();

Why not make the mutex recursive

It would work, and it would be the wrong shape. FinishOperation() reaches Cleanup(), which runs the caller's event handler and their completion callback. sessions_m_ was never meant to cover user code, and a handler that touches the client from inside it would deadlock against a lock it cannot see. Scoping the snapshot keeps the lock covering the container it was written for.

Testing it

curl_multi_perform cannot be made to fail from a test, so the case reaches resetMultiHandle() directly through a peer, the same way the OTLP exporters reach their private members. One registered session is enough: the snapshot picks it up, CancelSession() reaches the first re-lock and doRemoveSessions() the second.

I did carry a production failpoint while investigating, forcing the branch once with an injected counter, and that is how the report on #4389 got its stack. It is not in this change and I do not think it should be.

Against main with only the test added, the case does not fail, it hangs: timeout 45 gives exit 124 and zero cases finished. With the fix it passes in 501 ms. Worth knowing for a future bisect, since a timeout rather than a red assertion is what a regression here looks like.

What this does not fix

curl_multi_init() can return nullptr, and the current code stores that, after which every curl_multi_perform returns CURLM_BAD_HANDLE and lands straight back in resetMultiHandle(). Recovering from a failure to rebuild the multi handle is a policy question rather than a bug fix, so it is not here. #4391 covers the teardown ordering on the same path.

Checks

23 of 23 in curl_http_test, clean under OTELCPP_MAINTAINER_MODE=ON, clean under clang-format 18.1.8.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.26%. Comparing base (04a8ddd) to head (f1d1704).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4394      +/-   ##
==========================================
+ Coverage   82.18%   82.26%   +0.09%     
==========================================
  Files         501      501              
  Lines       19847    19847              
==========================================
+ Hits        16310    16326      +16     
+ Misses       3537     3521      -16     

Files with missing lines	Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h	95.00% <ø> (ø)
ext/src/http/client/curl/http_client_curl.cc	90.33% <100.00%> (+3.69%)	⬆️

🚀 New features to boost your workflow:

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

[lalitb]

Thanks.

[marcalff]

LGTM.

Thanks for the fix, and, more importantly, for all the analysis to identify this.

[marcalff]

label fix-merge-conflicts:

@thc1006

The conflict resolution HTML interface in github is good enough to resolve minor conflicts in CHANGELOG, so maintainers sometime resolve conflicts directly.

In this case I don't want to take risks and cause damages to your tests, so please take a look and rebase.

Thanks for the fix.

[thc1006]

label fix-merge-conflicts:

@thc1006

The conflict resolution HTML interface in github is good enough to resolve minor conflicts in CHANGELOG, so maintainers sometime resolve conflicts directly.

In this case I don't want to take risks and cause damages to your tests, so please take a look and rebase.

Thanks for the fix.

thank you for consider! I fix this now!!! thx for hard work reviewing!!!

[thc1006]

Rebased, and thank you for leaving it to me rather than guessing at the test file. The CHANGELOG half was the easy one, but the other conflict was two test cases landing on the same anchor, and taking either side would have quietly dropped one.

#4399 merging is what caused it. Both conflicts were against it: its CHANGELOG entry sits where mine wanted to go, and RetryJitterIsNotSharedAcrossThreads was inserted just above SendGetRequestSync, which is where this change adds its own case. Resolved by keeping both, in both files.

Same conflict on #4392 and #4395 for the same reason, so I rebased those two as well. All three are MERGEABLE again.

Checked after the rebase rather than assuming it came through clean: the lock is still scoped to the snapshot, the test peer and its friend are intact, opentelemetry/version.h is still included so include-what-you-use stays at zero on all three variants, and both test cases are present. 24 of 24 locally, clean under OTELCPP_MAINTAINER_MODE=ON and clang-format 18.1.8.

One thing worth flagging while you are here. All four of my open curl changes add their CHANGELOG line at the same place, which is why one merge broke three pull requests at once. If you would rather they went in a fixed order, tell me which and I will keep the others rebased behind it.