[thc1006]

Fixes #4404

Changes

Two allocation points whose failure was read as success. Each sits next to a check the same file already makes, which is what makes them look like oversights rather than a decision.

curl_slist_append

The docs name this pattern: "A null pointer is returned if anything went wrong, otherwise the new list pointer is returned. To avoid overwriting an existing non-empty list on failure, the new list should be returned to a temporary variable which can be tested for NULL before updating the original list pointer."

Assigning straight back over headers_chunk is the opposite of that. The pointer to everything appended so far goes with it, so the two places that free the list, Cleanup() and doRemoveSessions(), both see null and neither runs. Then the constructor carries on and dispatches Created, and Setup() only sets CURLOPT_HTTPHEADER when headers_chunk is non-null, so the request goes out with none of the caller's headers rather than not going out at all. For an OTLP export that is no Content-Type: application/x-protobuf and a receiver rejecting a request the exporter believes it sent correctly.

It appends into a temporary now, and on failure the partial list is released.

Reporting it is not the whole fix, which is the part I had wrong when I first opened this. Returning from the constructor does not stop Session::SendRequest: it calls SendAsync unconditionally, the easy handle is still valid, and Setup() only skips CURLOPT_HTTPHEADER rather than failing. So the first version of this announced CreateFailed and then sent the request anyway, without the caller's headers, which is the behaviour this PR exists to stop. Measured three runs out of three before the fix, one request reaching the test server each time.

The construction result is now held on the operation, Send() and SendAsync() both refuse on it before Setup(), and Session::SendRequest reports it once with the reason curl gave rather than the empty string that branch used to pass. The constructor also marks the operation terminal, or Cleanup() from ~HttpOperation announces a cancel for something that never started, to a handler the caller may already have dropped. That one aborts with pure virtual method called, and it only showed up once the send was actually stopped.

curl_multi_init

Same shape, three sites: both HttpClient constructors and resetMultiHandle(). The docs: "If this function returns NULL, something went wrong and you cannot use the other curl functions."

What a null one does, measured rather than assumed, against libcurl 8.14.1:

multi_add_handle(NULL, easy)    = 1  (Invalid multi handle)
multi_remove_handle(NULL, easy) = 1  (Invalid multi handle)
  CURLM_BAD_HANDLE              = 1

doAddSessions() drops that return, so such a client would accept every session, add none of them to a multi handle, and complete none of them. resetMultiHandle() is the one I would worry about most: it runs while recovering from a multi error, so a failure there turns a working client into a black hole for the rest of its life with nothing logged. All three now go through one small helper that logs.

HttpClient::wakeupBackgroundThread() already guards the same member for null before curl_multi_wakeup, so a null handle is something this file half expects. This makes it visible rather than silent.

The IO thread, once the handle is null

@lalitb asked whether logging the failure changes anything, since the client still takes requests. Following that down found two things it does not change.

A client built on a null multi handle does answer. curl_multi_perform rejects the handle, the loop creates a new one, the pending session moves to it, and the request completes. Measured three runs out of three, with the internal log captured in the same run so the numbers are not describing a healthy client. So refusing at SendRequest would trade a transient failure that repairs itself for a client that is permanently dead.

What does not recover is a handle that can never be created, and that used to be much worse than a missing feature. curl_multi_perform leaves still_running alone when it rejects the handle, and the loop initializes it to one, so the client kept reporting work it did not have: every pass took the still_running > 0 branch, which is above the is_shutdown_ check and above the wait. With the client alive, that cost 3.00 seconds of CPU and 1,168,126 error lines over a three second window, three runs out of three, and ~HttpClient could never join the thread.

Three changes close it. curl_multi_perform and curl_multi_info_read are not called with a handle libcurl refused to give, which is what curl_multi_init asks for. resetMultiHandle() reports whether the client has a handle afterwards. And a run of failures is reported once and waited on for as long as a poll would have taken. The same window now costs 0.00 seconds of CPU and two log lines.

Two things that reviewing the change turned up, rather than review. The wait cannot be interrupted, because wakeupBackgroundThread reaches the worker through the multi handle and the branch exists precisely because there is not one, so destroying a client whose worker was inside it measured 168 ms. It is taken in 16 ms slices with is_shutdown_ rechecked now, which measures 10 ms, five runs out of five. And curl_multi_poll and curl_multi_wait need no guard, since they sit inside the branch that only runs when perform succeeded. curl_multi_add_handle and curl_multi_remove_handle do get a null handle, and the section below is what became of that.

The handle was also created before libcurl was initialized

Members are initialized in declaration order, and multi_handle_ is declared before curl_global_initializer_, so both constructors called curl_multi_init before HttpCurlGlobalInitializer had called curl_global_init. libcurl asks for the opposite. No CI job could see it: the allocation cases install their allocators from SetUpTestSuite with curl_global_init_mem, which initializes libcurl before any client exists. The handle is created in the constructor body now, after every member including that one.

Tests

Both failures are covered now, and the first thing to say is that I was wrong above. I wrote that neither could be forced without an allocator hook. The hook is curl_global_init_mem, a documented part of libcurl, so that was a reason not to go looking rather than a reason it could not be done.

libcurl routes its internal allocations through the callbacks that function is given, so the suite installs a set of them and arms a thread local switch around the call under test. Thread local matters: once installed these callbacks serve the whole binary, and a client's background thread has to keep allocating normally while one test is arming a failure.

The switch is per allocation function rather than per call count, which is what keeps it independent of how many allocations a particular libcurl happens to make. Measured against 8.14.1:

	malloc	calloc	strdup
curl_easy_init	0	1	4
curl_slist_append	1	0	1
curl_multi_init	0	8	4

Failing malloc alone therefore selects the list append and leaves the easy handle intact, and failing calloc selects the multi handle. The header case also holds the reported reason to "Out of memory" rather than "Failed initialization", so it cannot pass on a curl_easy_init failure by accident, and it asserts what the fix is actually for: the server receives no request, and exactly one terminal outcome reaches the handler.

The flag those hooks set is std::atomic<bool>. It was a plain bool in the first version and the Bazel TSAN job reported the race, since libcurl calls the callbacks from whichever thread is allocating, including a client's background thread.

curl_global_init_mem returns CURLE_OK and quietly changes nothing once libcurl has been initialised, so the hooks go in from SetUpTestSuite. A third case asserts they were installed in time, since otherwise the other two would fail with a confusing message rather than a clear one.

Fail before and pass after, reverting one production change at a time to the shape main has. Re-measured on 54bf9523, since the commit the earlier version of this table named is on no remote ref any more. One run of the whole suite each, with the tree restored and rebuilt in between and the restored tree re-run to confirm it was back at 52:

reverted	failing
the curl_slist_append change only	3: AFailedHeaderAllocationIsReported, AHeaderListThatFailsPartWayThroughIsNotLost, ASynchronousSendRefusesAFailedHeaderAllocation
the curl_multi_init change only	2: AFailedMultiHandleAllocationIsReported, AFailedMultiHandleIsReportedForAnInstrumentedClient
nothing	none, 54 of 54 pass

Reverting the multi handle means deleting the helper as well as the two call sites, since an unused static function is an error under maintainer mode and a mutation that does not build shows nothing.

Checks

Measured on 9d15eb9e, this branch merged with main at ac7b87a8.

	result
curl_http_test, OTELCPP_MAINTAINER_MODE=ON, OTELCPP_WITH_STL=CXX14	54 of 54, no build warnings
the same suite with OTELCPP_WITH_STL=OFF	54 of 54, so dropping <memory> costs nothing off the CXX14 path
clang-format 18.1.8	clean on all five changed C++ files
clang-tidy and include-what-you-use	read from the CI jobs on this head rather than quoted here, because their limits moved from 73 and 76 to 204 and 218 while this sat and a local figure from before that does not compare

The suite is 52 where the tables above used to say 48, and none of that difference is this branch: main gained four cases in curl_http_test while this sat, InvalidRetryAfterUsesStableBackoff, RetryDeadlineIsReadyDuringResponseEvent, RetryDeadlineIsStableWithinAttempt and ShutdownDoesNotSpinWhileARetryIsQueued, and the rebase picked them up. The 16 cases this branch adds are the same 16. Three compression cases are compiled out in this configuration and one case is disabled on main, which is the gap between 56 in the file and 52 running.

Installed surface

Correcting myself on where these classes live, because I had it wrong here. ext/CMakeLists.txt installs four headers from ext/http/client, and http_client_curl.h and http_operation_curl.h are not among them: they are the implementation behind http_client_factory_curl.h. So a CMake consumer does not see either layout. Bazel is the other way round, since //ext takes hdrs = glob(["include/**/*.h"]), so a Bazel consumer can include both. That difference is its own question and not this one.

With that said, two layouts move. HttpOperation gains construction_result_, and HttpClient gains wakeup_generation_, an atomic the producers raise so the background thread can be woken without a multi handle. Both are private, and HttpClient::resetMultiHandle() becoming bool is private too. If the project holds these two to a layout guarantee anyway, say so: the construction result can be carried in the existing last_curl_result_ and session_state_, and the wakeup counter can go behind the existing multi_handle_m_ instead of being its own member.

What review turned up after that, and what came of it

Six things in the change itself, each measured before it was accepted and each with a case that fails without the fix, and five in the cases.

The worker could retire holding a request. Without a multi handle the add, remove and retry phases are skipped, and the retirement check read that as having nothing to do, so a request the client had already accepted could be sitting in pending_to_add_session_ids_ while the thread that was going to schedule it detached and left. Nothing brought it back, and FinishSession waited on a promise nobody was left to fulfil. How long that took depended on the libcurl the job was built against: the idle grace is a minute, but the line that sets it is behind a version check and older libcurl leaves it at zero, where the thread reaches the retirement check on its first idle pass. CMake asks for no minimum libcurl, so both are supported, and that is why the case was red on some jobs and green here. The thread now stays while it has work of its own left, with shutdown exempt, which is the whole difference from an earlier version of this gate that took every queue at its size and held the thread against the join in the destructor. Actionable is not the same as present, so an id whose session has gone is dropped rather than counted.

wakeupBackgroundThread did nothing without a handle. It goes through curl_multi_wakeup, and on libcurl older than 7.68.0 it compiled to nothing at all. Every producer now raises a counter first, and the wait taken when there is no handle watches that, so a queued request is picked up within a slice rather than at the end of the delay.

Three multi calls were still made with no multi handle. curl_multi_cleanup twice, in the destructor and in resetMultiHandle, and curl_multi_remove_handle through doRemoveSessions, which resetMultiHandle calls whether or not there is a handle. That is the contract the rest of this branch guards, so it should not have been the branch breaking it. Cleanup now goes through one function that answers whether there is a handle, reports a result that is not CURLM_OK, and leaves none behind. Detaching needs something to detach from, and there is nothing a null handle could name: the one it would have named was destroyed by curl_multi_cleanup, which detaches what it still holds, and nothing has been attached since, so the resource is released. Removing therefore no longer waits for a handle, since waiting held easy handles and header lists for the whole outage, which is the wrong way round.

The recovery cases were unsound. Both sent a request first, so the IO thread existed, and then called resetMultiHandle from the test thread, which destroys the multi handle and builds another while the IO thread may be inside curl_multi_perform, curl_multi_poll or curl_multi_info_read on the same one. libcurl leaves handles unsynchronized, and the IO thread does not hold multi_handle_m_ around those calls, so the lock the reset takes does not stand for it. Neither case needs to take a handle away: curl_multi_init is what fails in #4404, and it is called first by the constructor, so a client built while the allocator is refusing has no multi handle from the start and every attempt after that is the IO thread's own. Both are built that way now.

Resources could be left with no owner. An operation hands its easy handle and header list to pending_to_remove_session_handles_ on its way out, and that record is two raw pointers whose container frees neither. Everything that drains it runs on the background thread. Anything queued after that thread retired, or by the cancel the destructor itself does, had nobody left to release it, and the caller may already have had its terminal outcome: what is left over is the libcurl resources behind it. The destructor does that last pass itself now, before the multi handle goes so a handle still attached can be given back.

A retry queue entry outlived what it named. A retryable response puts the session in the retry queue with a time on it, and a cancel or a reset before that time left the entry there, since the session was still a session and still had an operation. When the time came the easy handle had gone back to the client, so libcurl was handed a null one and the result was never read, and until it came the idle check reported the queue as work. At shutdown that is the join waiting for an operation that finished long before: 6.4 seconds, three runs out of three. Entries are dropped when the operation is gone, was cancelled, or no longer holds an easy handle, and the remove and the add are read now, with the add only attempted if the remove worked and the operation finished rather than forgotten if it did not.

What the cases were getting wrong

Global state a failing case could leave behind. The allocator switches were armed with three fatal assertions between the arming and the disarming, so one of those returning would have left every later case in the binary allocating through a calloc that refuses, on every thread. The internal log handler is a process global that GlobalLogHandler reads and writes through a plain shared pointer with nothing synchronizing it, and one case put it back while the client, and the thread writing to it, were still there. Both are guards now, the log one declared before the client so it is destroyed after it. And curl_global_init_mem in SetUpTestSuite had no matching cleanup, so with every client taking one of its own through HttpCurlGlobalInitializer the count never reached zero and the allocator callbacks stayed installed into static teardown.

A count that did not say what it claimed. The log capture counted every line rather than the one the case is about, so anything else the binary wrote moved a bound that is meant to say how often one failure was reported. It takes the text it wants now. The allocation count had the same problem from the other end: the constructor's own attempt is one, so the assertion saying the IO thread had tried could be satisfied without the IO thread existing. Both cases put it back to zero after the constructor and before there is a thread to make one, and the thread that armed it is exempt from that point, so what is counted afterwards was refused to the IO thread and to nothing else.

A fix with no case that could fail without it. The header cases sent one header, so the append that failed was the first one and the list it was given was empty, which is the one shape the temporary pointer cannot be told apart from assigning the result straight back. Aiming at the second append needs the failure to land on a particular allocation, and the injection asked for any block of 64 bytes or less, which is a guess about a libcurl that promises none of it. It is measured now: the file watches one append happen and takes the size of the block it asked for, then refuses blocks of exactly that size after letting a stated number through. That also narrows what has to be skipped, from anything else having consumed the failure to an append not reaching the malloc callback at all. Against the assignment main has, LeakSanitizer reports 27 bytes in 2 allocations, three runs out of three.

The policy this picks, and the one it does not

A client with no multi handle can be answered two ways, and until now it was answered both, badly. This branch is recovery: work that has been accepted is kept, the IO thread stays while it owes somebody an answer, and the request goes out as soon as curl_multi_init succeeds, which the case measures at 1081 ms. Shutdown is the bound. There everything unscheduled is finished and the native resources are released, by the IO thread while it is still there and by the destructor when it is not. There is deliberately no retry budget: while the client is alive, a request it accepted keeps being tried.

Fail fast is the other coherent answer, a budget and then every pending operation finished exactly once, and it is a bound plus one condition on the same gate. @lalitb has already looked at this and said preserving the transient recovery is preferable to failing the client permanently, so recovery is what this does rather than an open question, and the fail fast shape is written down here only so that the choice is visible.

What was there before was neither. Recovery sometimes, a Cancelled nobody asked for sometimes, a request stranded until some later request happened to spawn a thread sometimes. Each of those is a commit here, and each has a mutation check: reverting the retirement gate hangs the queued case three runs out of three at the 240 second bound, reverting the destructor drain leaks 5582 bytes in 7 allocations, reverting the retry purge makes the destructor take 6.4 seconds, and reverting the temporary pointer in the header loop leaks 27 bytes in 2. The null handle guards are the one exception and are marked as such below: measured libcurl answers a null multi handle with CURLM_BAD_HANDLE rather than crashing, so what changes there is the contract rather than the behaviour.

What this does not close

Four things sit next to this and are not fixed by it. Saying so here rather than letting the branch read as if the whole client were now exactly-once.

curl_global_init's return is still ignored. This branch fixed the ordering, since multi_handle_ is declared before curl_global_initializer_ and member initialisation order meant curl_multi_init ran before curl_global_init had. Ordering is not success, and libcurl says a non-zero return means the other functions cannot be used. #4434 has the detail, including why there is no patch: the suite installs its allocator callbacks with curl_global_init_mem, which is the global initialisation, so by the time any client exists libcurl is initialised and the singleton's curl_global_init is a reference count bump that returns CURLE_OK whatever the allocator does. Reaching that failure needs a seam, and that is a conversation rather than something to slip in here.

Exactly-once is claimed for construction failure only. A Setup() failure still dispatches ConnectFailed from SendAsync and then CreateFailed from Session::SendRequest, which is #4360 item 3 and is untouched. What this branch does is give the constructor a result and make SendAsync refuse on it before Setup() runs, so a failed curl_easy_init or header list produces one terminal event. AFailedEasyHandleIsReportedOnce counts that and nothing wider.

The gzip branch still reports and sends. CreateFailed goes to the caller and then control falls through to SendAsync, carrying a body deflateInPlace may have written into, at its original length and with no Content-Encoding. #4360 item 1, untouched.

A direct HttpOperation caller is told by the return code, not by its handler. The constructor records a failure and does not dispatch it, so a failed curl_easy_init or header list reaches an EventHandler only through Session::SendRequest, once, as CreateFailed. That is coherent if Session is the only supported owner of asynchronous notification, and it is what keeps the count at one. The asymmetry is that the constructor does dispatch Created on its way out when it succeeds. Making it symmetric means the constructor reports and Session::SendRequest then must not, which is more machinery for the same single event, so I have left it as it is and would rather be told. How much that matters depends on which build you take. ext/CMakeLists.txt does not install http_operation_curl.h, so a CMake consumer cannot construct one directly at all, while //ext takes hdrs = glob(["include/**/*.h"]) and a Bazel consumer can. So the question is narrower than an installed API and wider than an internal detail, and it is still yours to answer rather than mine.

What the cases cover, and what they do not

Against the failure schedules worth having, since a list of green cases says less than a list of the ones that are missing.

Covered here: a multi handle that cannot be created and then can; one that never can, through to shutdown; a removal queued with no multi handle, and one still queued when the client goes; a retry entry whose session was cancelled before its time; the first header append succeeding and the second failing; a failed curl_easy_init on both the synchronous and asynchronous paths. Covered on #4395: a session a reset took before its id was queued, and one curl_multi_add_handle rejects.

Also covered, and added because the list of failure schedules worth having asked for it: a client destroyed with a retry still pending and nobody cancelling it, which is what an exporter shutting down actually does. Both retry cases fail against a queue check that only drops entries with no operation, three runs out of three, at 5.40 seconds against a bound of four.

Not covered, and each for a reason rather than an oversight: a working multi handle failing mid-transfer, because curl_multi_perform cannot be made to fail from a test without a seam; a retryable response already queued when a reset happens; a curl_easy_setopt failing inside Setup(), which is the #4360 path above; and the gzip failure, likewise. The libcurl version split is covered rather than skipped, because the queued case pins the zero idle grace that older libcurl uses, so both branches of that version check run on every platform.

Configurations, because one build saying yes is not the same as the matrix saying yes. Re-measured on 9d15eb9e, each one configured and built on its own and the whole binary run alone and once, on Linux with gcc 14 and libcurl 8.14.1. The earlier version of this table gave 40 and named no commit, which made it uncheckable; the counts below are 52 for the reason the Checks section gives, that main grew this file by four cases while this sat.

	cases
default, OTELCPP_WITH_STL=CXX14	54	pass, 0 build warnings
OTELCPP_WITH_STL=OFF	54	pass, 0 build warnings
OTELCPP_WITH_STL=CXX20	54	pass, 0 build warnings
thread instrumentation preview on	54	pass, 0 build warnings
compression preview on	57	pass, and the three gzip cases only exist here
AddressSanitizer, ASAN_OPTIONS=detect_leaks=1	54	pass, nothing reported
ThreadSanitizer	54	pass, nothing reported
UndefinedBehaviorSanitizer, -fno-sanitize-recover=all	54	pass, nothing reported

The retry-preview-off row is gone rather than renumbered: #4668 removed the flag, so that configuration cannot be built.

A sanitizer that was never linked in reports nothing in the same way one that finds nothing does, so each of those rows is checked rather than assumed: nm counts 30 __tsan_ symbols in the ThreadSanitizer binary, 35 __asan_ in the AddressSanitizer one and 13 __ubsan in the third, and none of them appear in the other six. The compression row is checked the same way, by the gated case names being present in that binary and absent from the rest, and the STL rows by reading OTELCPP_WITH_STL back out of each cache. I did not repeat the deliberate-leak control for LeakSanitizer on this run, so that row stands on the instrumentation check alone. Two things that came out of running the rest rather than assuming them: with compression on there are two more cases that had never been compiled in anything I ran, and ThreadSanitizer found two data races, both of them in a test peer of mine that read the retry queue from the test thread while the background thread pushed to it. That queue has no lock because only the background thread is meant to touch it, so the peer made its own premise false. It is gone, and the cases wait on what the server received instead.

Not run, and worth saying: Valgrind, a no-exceptions build, static against shared libcurl, Windows and macOS, and libcurl older than 7.68. The last one is the only one that worries me and it is the one the cases work around, by pinning the zero idle grace that older libcurl uses so both sides of that version check run everywhere.

Related

#4395 is a dependency rather than an overlap, and this section used to say the opposite. resetMultiHandle keeps the sessions whose ids are already in pending_to_add_session_ids_ and takes the rest, and a request between CreateSession and SendAsync is one of the rest: the reset cancels it, and on main the id then goes into the queue whatever became of the session, where doAddSessions finds nothing to add and moves on, leaving a promise nobody fulfils. #4395 refuses that id and finishes the operation itself, and ASessionResetTookBeforeItWasQueuedIsFinished on that branch is the case for it: it hangs three runs out of three against a revert, and passes in 503 ms. So this branch can recover a multi handle and still leave that one request unanswered without it, and the order that works is #4395 first with this rebased on it.

#4405 answers the ownership question this one raises, and the two do not combine mechanically. The ledger there records whether the multi handle was ever given a particular easy handle, which is the question a missing handle makes unanswerable from multi_handle_ alone, and a refused removal is kept in a member queue rather than dropped. The resolution that works puts attached_handles_.clear() inside the single function that releases the multi handle, so a ledger naming a handle that has gone is not a state either branch can reach. I built the two together to find out rather than assume: AResetThatCannotBuildAReplacementLeavesNothingAttached is the combined case, and without that one line it reports the handle still attached and the record quarantined for a multi handle that is never coming. The combined tree passed 46 of 46 under AddressSanitizer with detect_leaks=1 when I built it. Both branches have been rebased since and they no longer merge on their own: CHANGELOG.md, http_client_curl.cc and curl_http_test.cc all conflict. The resolution above is still the one I would reach for, but the run behind that count cannot be reproduced from either head, so the integration is worth redoing against whichever of the two lands first.

Small overlaps with two open PRs, both one line, and I will take the rebase rather than ask anyone else to:

• [BUG] Remove a curl easy handle before releasing what it points at #4405 makes the same include change, because it also logs from outside the compression guard: one unconditional global_log_handler.h in place of the conditional copy.
• [BUG] Scope the session lock to the snapshot in resetMultiHandle #4394 has landed, and the rebase keeps its change rather than the old body: sessions_m_ is still scoped to the snapshot inside resetMultiHandle(), and friend class HttpClientTestPeer is still on the client.

#4392 has landed and this branch is rebased on it, and its rule is kept: nothing here writes to an easy handle from the calling thread. The one conflict was in the test file, where both add cases at the same place, and both sets are present. #4394 is in the base too, and the sessions_m_ scope inside resetMultiHandle is still the snapshot only.

#4391 asked for the unchecked curl_multi_remove_handle and curl_multi_add_handle, and the retry path is one of the places it named. That pair is read now, and the add only happens if the remove worked, so that part of it is answered here. The rest of #4391 is the teardown ownership #4405 carries, and the issue should stay open for it.

About the lines Codecov marks

Codecov reported 11 lines of this change without cover. I read them rather than
adding a test per line, and they are not one thing.

Three of them were the TLS version range and the cipher list in Setup(), and
they were uncovered because nothing in the suite passed either option. Two cases
now do, and gcovr reports one hit each afterwards. The first draft of those
cases passed while covering none of it, because the whole SSL block sits behind
ssl_options_.use_ssl and I had set the TLS fields without it; the measurement
is what caught that, not the green.

The rest are in the recovery path this change adds for a null multi handle, and
in the two curl_multi_cleanup failure logs. Reaching the first needs a client
whose multi handle failed to initialise, and there is no seam for that here:
curl_multi_init is called directly. Reaching the second needs
curl_multi_cleanup to fail, which it did not in 104 and 13 evaluations
respectively. I have left both uncovered rather than build injection machinery
for them, and I would rather say so than have the number read as an oversight.

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 90.00000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.91%. Comparing base (ac7b87a) to head (9d15eb9).
⚠️ Report is 2 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4406      +/-   ##
==========================================
+ Coverage   86.69%   86.91%   +0.22%     
==========================================
  Files         527      527              
  Lines       20902    20993      +91     
==========================================
+ Hits        18119    18243     +124     
+ Misses       2783     2750      -33     

Files with missing lines	Coverage Δ
...ntelemetry/ext/http/client/curl/http_client_curl.h	95.13% <ø> (ø)
ext/src/http/client/curl/http_operation_curl.cc	71.74% <83.34%> (+6.81%)	⬆️
ext/src/http/client/curl/http_client_curl.cc	93.16% <90.36%> (-0.94%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

[thc1006]

Both red jobs were mine, and one of them was a wrong assumption rather than a flake.

The two conan jobs. AFailedHeaderAllocationIsReported assumed a failing malloc reaches curl_slist_append, which is what happens on libcurl 8.14.1, where I measured it. On the libcurl those jobs build, curl_easy_init takes the allocation first, prints Curl_open failed, and the request leaves down a different path: terminal_count_ is 3 rather than 1 and the reason is not "Out of memory". Which function uses which allocator is not part of libcurl's contract, so the case should never have rested on it.

The failure is bounded by size now. A curl_slist node is two pointers and a Curl_easy is thousands of bytes on every version, so the bound aims it at the list append. That is not a guarantee either, so the reason check became a GTEST_SKIP that names the allocation that actually failed rather than an assertion against a path the build never took. Locally the bound still selects the list append on 8.14.1 with no skips, and the case still fails three runs out of three with the production change reverted.

The IWYU job. The file already included curl/curl.h inside the ENABLE_OTLP_RETRY_PREVIEW guard, and the scaffolding calls curl_global_init_mem from every configuration. So all-options-abiv1, where that guard is off, asked for the include. Adding an unconditional one then made both preview variants ask for the guarded copy to go. One unconditional include and no guarded copy, and all three variants report correct includes. Reproduced locally against the same cache files with CMAKE_CXX_STANDARD=14 and WITH_STL=CXX14, which is what ci/do_ci.sh cmake.iwyu.test passes on top of them.

27 of 27 in curl_http_test, three runs out of three, and clang-tidy at zero delta for that file under the CI filters.

Edited 16 Aug. The size bound described above did not hold either. It let one allocation of a node's size through and refused the next, and on the libcurl the Conan jobs build something else consumed the refusal, so the list was built, the request went out, and AHeaderListThatFailsPartWayThroughIsNotLost failed on all three of them while the same commit passed the other 68 jobs. A node's size is not part of libcurl's contract and neither is what gets allocated before the header list, so the injection is keyed on content now: the case names the header to refuse and the refusal happens when libcurl copies that exact string. All three jobs are green. Leaving the original text because the reasoning that led to the size bound is the same reasoning that had to be given up.

[lalitb]

// built on one accepts sessions and never sends any of them. Say so rather than fail quietly.

This no longer matches the demonstrated behavior: the I/O loop can repair a transient initialization failure and complete the request. Could you update this comment and the matching test comment above AFailedMultiHandleAllocationIsReported?

[thc1006]

Thank you, and sorry for the slow reply. You were right, and the comment came out in d9034527. What stands there now only claims what that function knows:

// Reported once, where it happens. The IO loop does its own reporting, because sharing this one
// would repeat the same line on every pass for as long as the handle stays missing.

Please close this thread if that reads right to you.

[thc1006]

The Codecov patch report is answered rather than argued with. Both of the lines it named are reachable, and one of them turned out to be a real hole rather than a coverage artifact.

Send() carries the same construction_result_ guard as SendAsync, and nothing in the suite called it after a failed construction. ASynchronousSendRefusesAFailedHeaderAllocation builds an operation whose header list cannot be allocated and calls Send(). Reverting the guard and running it again is what makes the point: the request goes out carrying none of its headers, the embedded server records it, and the case fails, five runs out of five.

with the guard:     result=27(Out of memory)  requests_seen=0
without the guard:  result=0(No error)        requests_seen=1

The other line is the multi handle in the constructor that takes thread instrumentation. Both constructors reach it through the same helper, so AFailedMultiHandleIsReportedForAnInstrumentedClient uses that overload with a null instrumentation and holds that it reports a handle it could not create.

Measured with the same all-options-abiv2-preview configuration ci/do_ci.sh code.coverage uses, intersecting the uncovered lines with the lines this branch adds: three missing before, none after. 32 cases in curl_http_test, three runs out of three.

[lalitb]

I’m still trying to understand this part. If recovery is broken, the checks above fail, but we still call FinishSession(). Wouldn’t that hang because the request never completed? Should we cancel the active session here so the test can fail normally?

[thc1006]

You are right, and following it down found something that is not in the case at all.

I gated resetMultiHandle out to check, and the case does what you say: both assertions fail and print, and then it hangs. The stack says the hang is not where either of us assumed.

Thread 1 (main):
  #4 std::thread::join()
  #5 HttpClient::~HttpClient() at http_client_curl.cc:327

Thread 2 (IO):
  #4 HttpClient::doAbortSessions() at http_client_curl.cc:776   <- running, not blocked

FinishSession returns. What blocks is the client destructor joining a background thread that never exits, and the reason is a value the failed call did not write. curl_multi_perform leaves still_running alone when it rejects the handle, and the loop initialises it to one, so a client with a null multi handle keeps reporting work it does not have. Every pass takes the still_running > 0 branch and continues, which is above the is_shutdown_ check and above the wait, so the thread spins a core and can never be joined.

That is reachable without any test gate. Making curl_multi_init fail persistently, which is what a real allocation failure looks like, hangs the whole test binary: twice out of twice at a sixty second timeout, and again at two minutes for the full suite.

Clearing still_running on that branch is the fix, and it is in this change's scope since it is the reset's own curl_multi_init result going unchecked one layer up:

          if (mc != CURLM_OK)
          {
            still_running = 0;
            self->resetMultiHandle();
          }

With it, the persistent failure finishes in 521 ms, three runs out of three, and the case passes rather than hangs, because the reset cancels the session and the caller is told. A handle that fails once and then works is unaffected: the request still completes with a response, three runs out of three with the internal log confirming the first handle really was null.

And I took your suggestion for the case itself. It cancels the session before finishing it, so a failure ends the case instead of leaving FinishSession waiting on an operation nothing is going to complete.

30 cases in curl_http_test, three runs out of three.

[thc1006]

Half of this is now upstream, so the scope has shrunk. Rebased on main at ac7b87a8 and narrowed to what is left.

The security fix in ae1b7a4f added its own curl_slist_append null check, reporting through Setup()'s return rather than from the constructor. I took main's version in both conflicts: it uses the path Send() and SendAsync() already consult, ConnectFailed is pinned by main's own RejectUnsafeHeadersBeforeSyncOrAsyncTransfer, and header_setup_result_ and my construction_result_ were one field under two names. Three of my test cases went with the code they covered, since main brought its own.

What is left is the other half of #4404. curl_multi_init is still unchecked at three sites on main, at http_client_curl.cc:299, :310 and :934, and a client built on a null multi handle still accepts sessions it can never complete. The easy handle case stays too, with one change: it records the failure instead of dispatching from the constructor, because Setup() would otherwise call curl_easy_setopt on a handle that does not exist.

Measured on 69b4e6d4, OTELCPP_WITH_STL=CXX14 with maintainer mode: 53 of 53 pass, no build warnings, and that includes the three cases ae1b7a4f brought. clang-format 18.1.8 is clean on all changed files.

One thing came out of the rebase that is not about this branch. Two of my cases assert exactly one terminal outcome and started failing against today's main. They were right: the new header validation reaches two, ConnectFailed from SendAsync and then CreateFailed from Session::SendRequest for the same failure. That is item 3 of #4360, now reachable from an ordinary request rather than only from an allocation failure, and I have put the reproduction there.