[thc1006]

Fixes #4391.

What was wrong, and one thing I had wrong about it

The issue has three parts: Cleanup() writes to an easy handle that may still be transferring, doRemoveSessions() frees the header list before it takes the handle out of the multi handle, and neither the removal nor the retry path reads the CURLMcode it gets back. The first two are ordering and they're fixed by moving two calls.

The third one I got wrong in my own comment on the issue, where I said a handle the multi handle never took reports CURLM_OK, so nothing has to remember which handles were added. That's true of the libcurl I tested on and it isn't true in general. The prologue of curl_multi_remove_handle picked up a second condition for two minor releases:

libcurl	prologue	never added, multi handle empty
7.81.0, 8.5.0, 8.6.0, 8.7.1, 8.8.0, 8.9.1	if(!GOOD_EASY_HANDLE(data))	CURLM_OK
8.10.1, 8.11.1	if(!GOOD_EASY_HANDLE(data) || !multi->num_easy)	CURLM_BAD_EASY_HANDLE
8.12.0, 8.14.1	if(!GOOD_EASY_HANDLE(data))	CURLM_OK

find_package(CURL) here has no lower bound, so both are supported. And the version window isn't even needed to reach the same branch: I measured curl_multi_remove_handle(NULL, easy) returning CURLM_BAD_HANDLE on 8.14.1, which is what a client whose curl_multi_init() failed hands to every removal.

What made that reading dangerous is what the failure branch did with it. pending_to_remove_session_handles_ is swapped into a local, so a continue past a record ends the life of that record at the end of the pass. The easy handle and its header list go with it, and so does the shared_ptr<Session> the same pass was releasing, while the handle still carries that session in CURLOPT_PRIVATE. The message loop reads it straight back:

curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &session);
const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr;

What this does now

• An easy handle is recorded when curl_multi_add_handle accepts it, and unrecorded when curl_multi_remove_handle gives it back. A handle that was never accepted is freed without asking the multi handle anything, so neither libcurl's answer nor the version it came from decides the path.
• A removal libcurl refuses keeps the whole record: handle, header list and the session it names. Those are freed after curl_multi_cleanup has retired the multi they were on, in resetMultiHandle() and in the destructor. Cleanup is not what detaches them: multi.h is explicit that it does not free or touch any individual easy handle. What makes the order safe is that the multi is gone by the time they are freed. No retry loop, because a refusal here doesn't become acceptance later, and I didn't want a busy path in the IO loop.
• The queue holds one record per easy handle rather than one per session. A session that starts another request hands over a second handle while the first is still queued, and the map keyed by session dropped the first one.
• The destructor releases what the background thread never got to. That path frees nothing today: a handle handed over after the last pass measured 5594 bytes in 7 allocations under LeakSanitizer.
• doRetrySessions() goes through the same accounting and reports a re-arm libcurl won't accept, which is the silence the issue asks about.

Evidence

Fail before, pass after, against this branch with the change reverted and the two scenarios rewritten against the members that existed before it. The commit I ran that on has been rebased away since, so reproducing it means doing the revert rather than fetching a sha:

	before	after
ARefusedRemovalKeepsTheHandleAndTheSessionItNames	fails, the session is released while the handle still names it	passes
ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst	fails, 1 record queued where 2 were handed over	passes
LeakSanitizer on those two	11171 bytes in 14 allocations	0

Each case was then checked against the mutation it's meant to catch, on an otherwise clean tree:

mutation	case that fails
a refused removal drops the record again	ARefusedRemovalKeepsTheHandleAndTheSessionItNames
ask libcurl even about a handle it was never given	AHandleTheMultiHandleNeverTookIsFreedNotStranded
drop this session's queued handles on a new add	ASecondHandleFromTheSameSessionDoesNotDisplaceTheFirst
forget to unrecord what the removal gave back	TheAttachmentLedgerIsEmptyOnceARequestFinishes
forget to record what the add accepted	nothing

That last row is honest rather than tidy. With the record never written, every release takes the never-added path and frees a handle the multi handle still holds, and I could not get that to show as anything: curl_easy_cleanup detaches the handle itself on the way out, so the state afterwards is identical either way. Measured on 8.14.1 under ASan and UBSan, adding a handle to a multi and performing until running_handles is 1, then freeing it while it is still attached:

add_handle    = 0
perform       = 0, running=1
easy_cleanup done
perform after = 0, running=0

The multi drops it. Freeing in the other order, curl_multi_cleanup first and then the attached handle and its header list, is also clean on that build. So the half of the accounting that's covered is the half that skips a removal it doesn't need, and the half that isn't is the documented call order, which I have not found a way to make bite on this libcurl.

One case fixed that isn't about any of this

The valgrind job went red on RepeatedCallerThreadCancelsAreClean, which came in with #4392 and has nothing to do with the accounting here. It sums Cancelled and Response over twenty attempts against a closed port and asks for at least one each, and there's a third way such an attempt ends: the connection fails before the cancel arrives, and the cancel is then correctly a no-op. Instrumented, the silent attempts run Created, Connecting, ConnectFailed and stop there.

Pinned to one core, fifteen runs each:

	pinned to one core	unpinned
main at 3fb1d317	15 of 15 fail, 3 to 7 cancels of 20	15 of 15 pass
#4395, at the head it had then	15 of 15 fail, 3 to 7	12 of 12 pass
this branch before the change	15 of 15 fail, 3 to 7	12 of 12 pass
this branch after it	15 of 15 fail, 4 to 7	12 of 12 pass

Same on all four, so it isn't the change, it's a case that pinned one of two legitimate outcomes. It's now asking each attempt to end somewhere the handler is told about rather than to end the same way twenty times, and pinned to one core that goes from 15 of 15 failing to 0 of 15. The connection failure is counted apart from terminal_count_, which two other cases pin to an exact value, and outside the existing chain, because ACancelBeforeTheResponseReportsCancelled cancels from ConnectFailed and needs that branch to keep firing.

It's a separate commit. It rides along because it's the job this PR makes red and it's a case I added, but it splits out cleanly if you'd rather have it on its own.

Checks

Measured on 7e6ed213, this branch merged with main at ac7b87a8.

	result
curl_http_test	50 of 50
the same under ASan with detect_leaks=1	50 of 50 on each of five runs, no sanitizer output
bazel test //ext/test/http:curl_http_test	passed in 29.7 s
clang-format 18.1.8	0 findings on the changed C++ files

The suite is 46 rather than the 36 an earlier revision of this section reported,
because #4668 removed WITH_OTLP_RETRY_PREVIEW and the cases that flag used to
gate now run in every build. That also retires the second configuration this
table used to carry, since it can no longer be set. clang-tidy and
include-what-you-use have CI jobs on every head and are not repeated here; their
limit moved from 73 to 204 while this sat, so a local figure from before that
would not compare.

Related

Rebased on current main. #4395 also touches Cleanup() and is still open, and this revision doesn't go near it: the accounting is all in http_client_curl.cc and its header, and the Cleanup() hunk is the one the first commit on this branch already had.

About the line Codecov marks

Codecov reported 5 lines of this change without cover. One of them was the CA
certificate blob, uncovered because nothing in the suite supplied a certificate
as a string, and a case now does: gcovr reports one hit afterwards. It sets
use_ssl, without which the whole SSL block is skipped and the case would pass
having exercised none of it.

The others are in the ledger's re-attach path and in a log on a branch that was
evaluated 34 and 62 times without being taken. Reaching them needs the removal
and the re-add to interleave in a particular order, which is a timing window
rather than a call, so I have left them uncovered rather than write a case that
would pin a schedule.

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

❌ Patch coverage is 94.11765% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.87%. Comparing base (ac7b87a) to head (7e6ed21).

Files with missing lines	Patch %	Lines
ext/src/http/client/curl/http_client_curl.cc	93.98%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4405      +/-   ##
==========================================
+ Coverage   86.69%   86.87%   +0.19%     
==========================================
  Files         527      527              
  Lines       20902    20957      +55     
==========================================
+ Hits        18119    18205      +86     
+ Misses       2783     2752      -31     

Files with missing lines	Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h	95.13% <ø> (ø)
...lemetry/ext/http/client/curl/http_operation_curl.h	100.00% <ø> (+9.10%)	⬆️
ext/src/http/client/curl/http_operation_curl.cc	69.64% <100.00%> (+4.71%)	⬆️
ext/src/http/client/curl/http_client_curl.cc	94.37% <93.98%> (+0.27%)	⬆️

🚀 New features to boost your workflow:

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

[thc1006]

Where the five uncovered patch lines are, measured with the same all-options-abiv2-preview configuration ci/do_ci.sh code.coverage uses rather than read off the Codecov page.

One is the guard this change is about. PerformCurlMessage returns early when is_cleaned_ is set, for the message the multi handle had already buffered when the operation was torn down. Reaching it needs a CURLMSG_DONE to arrive for an operation that has been cleaned up in the same pass, which is the window the change exists to close. Nothing in the suite creates it today, and the case that would has to cancel a transfer that is already in flight and then land inside one loop iteration, so it is a timing case rather than a deterministic one.

One is a null easy handle in doRemoveSessions. Every path that schedules a removal today moves a live resource in, so a null handle in that map is the defensive half of the check rather than a state the client produces.

Three are the curl_multi_remove_handle failure branch. That call returns CURLM_OK for a handle that was never added, which is the whole reason the change can remove unconditionally, so the failure needs a handle owned by a different multi handle or a null multi handle. Both are reachable through the HttpClientTestPeer friend that #4394 added, and neither happens on its own.

I would rather say that plainly than add cases that pin defensive branches into place with test-only seams. If you want any of the three covered, say which and I will send them.