[Dyfintie]

Fixes #4434.

## Changes
- [x] Bug fix (non-breaking change which fixes an issue)

This PR addresses the unhandled return value of `curl_global_init(CURL_GLOBAL_ALL)` in `HttpCurlGlobalInitializer`:
- Stores the return `CURLcode` and tracks an `is_initialized_` boolean status.
- Logs an error with `OTEL_INTERNAL_LOG_ERROR` if `curl_global_init` fails.
- Skips calling `curl_global_cleanup()` in the destructor if initialization failed.
- Avoids creating `multi_handle_` in `HttpClient` if global init failed.
- Immediately signals `SessionState::CreateFailed` on `HttpClient::SendRequest()`, `HttpClientSync::Get()`, and `HttpClientSync::Post()`.
- Adds unit tests in `curl_http_test.cc` via `HttpClientTestPeer` covering both asynchronous and synchronous client behaviors on

initialization failure.

## Testing
- Added unit tests:
  - `BasicCurlHttpTests.GlobalInitFailureHttpClient`
  - `BasicCurlHttpTests.GlobalInitFailureHttpClientSync`
- Ran full test suite locally: all 28 tests in `curl_http_test` passed.
- Formatted with `clang-format`.

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

* [ ] `CHANGELOG.md` updated for non-trivial changes
* [x] Unit tests have been added
* [x] Changes in public API reviewed

[thc1006]

Thanks for this, and for asking on the issue first. Two things I checked that I think are right, and one question.

The guard in Session::SendRequest is in the right place, and it is worth saying why, because it is easy to lose in a later edit. It is the first statement in the function, so it returns before http_client_.MaybeSpawnBackgroundThread(). That matters: the background loop calls curl_multi_perform, curl_multi_poll, curl_multi_wait and curl_multi_info_read on multi_handle_ with no null check, so the thread never starting is what keeps those safe rather than any check inside them.

Moving the curl_global_initializer_ declaration up in the header is also right, and necessary rather than cosmetic. Members initialise in declaration order and not in the order of the initialiser list, so without the header change the new multi_handle_ initialiser would read a member that had not been constructed yet.

The question. On main there are four more uses of multi_handle_ that are not null checked: curl_multi_add_handle around line 752, curl_multi_remove_handle around 819 and 863, and the curl_multi_cleanup and curl_multi_init pair around 913. Reading the code I could not reach any of them from a failed global init, because each sits behind either the background thread or a session that was already scheduled, and this PR stops both. curl_multi_wakeup is already guarded on main. Since this PR is what makes a null multi_handle_ possible in the first place, it seems worth confirming that rather than inferring it, and a case that constructs the client with a failed initializer and then calls CancelAllSessions() and FinishAllSessions() would cover the destructor path.

One overlap to be aware of rather than to act on. #4406 makes the client survive without a multi handle from the other direction, where curl_multi_init() itself fails, and it measured what the current code does in that state: 3.00 seconds of CPU in a 3 second window and 1,168,126 error lines from the IO thread. Same end state, different trigger. It is a draft and parked, so please do not wait for it. I will rebase onto whichever of us lands first.

Small thing: "a private constructor to to simulate" in the header has a doubled "to".

[Dyfintie]

Thank you so much for the thorough review and insights :)
I have updated the PR with the following changes:

1. Guard rationale comment: Added a comment in
   Session::SendRequest documenting that returning early before
   MaybeSpawnBackgroundThread() prevents the background IO loop
   from starting with an uninitialized or null multi_handle_.

2. Explicit test coverage: Extended BasicCurlHttpTests.
   GlobalInitFailureHttpClient to explicitly call client.
   CancelAllSessions() and client.FinishAllSessions(), ensuring the
   cancellation and destructor paths execute cleanly when
   multi_handle_ is null.

3. Comment typo fix: Fixed the doubled "to" in the header
   comment.:)

4. Also fixed implemented the linter fixes.

[thc1006]

All three are done, and I checked each rather than taking the summary for it.

The guard comment says the thing that matters, that the early return happens before MaybeSpawnBackgroundThread(). The test now injects the failed initializer, asserts IsValid() is false, sends a request, and then calls CancelAllSessions() and FinishAllSessions() with multi_handle_ null, and the client is a stack object so the destructor runs on the same path. That is what I was asking to see confirmed rather than inferred. The sync case covering Get and Post returning CreateFailed was not something I asked for and is a good addition. The doubled "to" is gone.

Worth flagging separately, because you cannot see it from your side and it is not something you did wrong: no CI has run on 2c5efd26. The pull request shows only the EasyCLA check, and the API says there are eight GitHub Actions check suites sitting at conclusion: action_required:

gh api repos/open-telemetry/opentelemetry-cpp/commits/2c5efd26/check-suites \
  --jq '.check_suites[] | "\(.app.name) \(.status) \(.conclusion)"'
GitHub Actions completed action_required     (x8)

action_required is the approval gate for a first contribution: workflows do not start until a maintainer presses "Approve and run workflows", and it applies again to each new push until you have something merged. CI did run on your earlier head, which is how I saw the MSVC red last week, so this started when you pushed the fixes above.

So the pull request is not being ignored, it is unevaluated. @marcalff or @lalitb, could one of you approve the workflow run on this one? The change is small, EasyCLA has passed, and it closes #4434.

[thc1006]

The workflows ran, and the 31 red checks are one compile error rather than 31 problems. It is a one line fix and you could not have hit it locally, which I will explain.

CI reports this:

http_client_curl.cc:56:29: error: invalid operands of types 'const char [61]' and 'int' to binary 'operator<<'

That is the second error, not the first. Reproducing it locally gives both:

error: invalid operands of types 'const char [61]' and 'int' to binary 'operator<<'
error: 'OTEL_INTERNAL_LOG_ERROR' was not declared in this scope

The macro is not declared, so the compiler reads OTEL_INTERNAL_LOG_ERROR(...) as a call and tries to evaluate its argument, and "a string" << an int on its own is what it then complains about. The << is fine.

Why it is not declared: the include sits inside the #else of a guard, near the top of the file.

#ifdef ENABLE_OTLP_COMPRESSION_PREVIEW
...
#else
#  include "opentelemetry/sdk/common/global_log_handler.h"
#endif

The one log call that existed before yours is at line 217, inside the same #else, so it never needed the header anywhere else. Your new call at line 55 is outside the guard, so with compression preview on the header is never included.

Moving the include out of the #else fixes it:

#ifdef ENABLE_OTLP_COMPRESSION_PREVIEW
...
#endif

#include "opentelemetry/sdk/common/global_log_handler.h"

Measured on your branch, building //ext/src/http/client/curl:http_client_curl:

	before	after
default	builds	builds
--copt=-DENABLE_OTLP_COMPRESSION_PREVIEW	2 errors	builds

And why your local build was green: compression preview is off by default, which is the half that already had the header. A single local configuration cannot see this one. The three IWYU jobs and the two clang-tidy jobs are red for the same reason, since neither tool can run on a translation unit that does not compile, though I would not promise they all clear until CI says so.

Sorry this cost you a round trip. The guard is genuinely easy to miss and it has caught me in this same file before.

[Dyfintie]

Thank you so much for the detailed explanation and diagnosis, @thc1006 ! That makes complete sense.
I have moved #include "opentelemetry/sdk/common/global_log_handler.h" out of the #else branch so that it is included unconditionally across all configurations. I also verified the build locally both with and without -DENABLE_OTLP_COMPRESSION_PREVIEW to ensure both compile cleanly.
Rebased and force-pushed to the branch!

[thc1006]

Checked b6dd266f here rather than going by the summary, and the move does what we hoped.

	result
default	builds
--copt=-DENABLE_OTLP_COMPRESSION_PREVIEW	builds

I also put the include back inside the #else on your head, so that the pass is attributable to the move and not to something else that changed on the branch:

error: invalid operands of types 'const char [61]' and 'int' to binary 'operator<<'
error: 'OTEL_INTERNAL_LOG_ERROR' was not declared in this scope

The same two errors come back, and they go away again when it comes back out. Your change is the one doing the work.

One thing that has nothing to do with your code, and is worth knowing so the quiet week does not read as something you missed. All eight workflow runs on this head are sitting unstarted:

CI                    action_required
clang-tidy            action_required
include-what-you-use  action_required
cppcheck              action_required
CodeQL                action_required
CMake Install Tests   action_required
doxygen-tidy          action_required
Zizmor                action_required

action_required means the run is waiting for someone with write access to start it, and these have been waiting since your push on 18 September. Nothing in the branch causes it and neither of us can clear it from where we sit. The branch has also fallen behind main since then, which is the easy part and can wait until someone is ready to look.

Thanks for sticking with this one. The guard that caused it is genuinely easy to walk into, and the fix reads well now.

[Dyfintie]

Thanks a lot for the detailed verification and explanation, @thc1006!
I have rebased the branch onto the latest main and resolved the merge conflict in Session::SendRequest (with #4431). The initialization validity check is now placed cleanly before the send_started_.exchange(...) guard.

All unit tests pass locally. Ready for the workflows to be approved and run whenever maintainers have a moment!

[thc1006]

Checked 0c7004f8 here, and the ordering you picked is the one worth having.

  if (!http_client_.IsValid())
  {
    ...
    return;
  }

  if (send_started_.exchange(true, std::memory_order_acq_rel))

The validity check sits above the exchange, so a failed curl init reports CreateFailed without consuming the one shot. Had it gone the other way the session would be spent on an attempt that never started, and nothing would say why. That is a subtle one to get right on a rebase.

Built and ran it:

	result
//ext/src/http/client/curl:http_client_curl	builds
same with --copt=-DENABLE_OTLP_COMPRESSION_PREVIEW	builds
//ext/test/http:curl_http_test	passes

The workflows are still waiting. All eight runs on this head are at action_required, same as before the rebase, so the approval is per push rather than per branch and a maintainer needs to start them again. The branch is also one commit behind main now, after #4632 landed, which is nothing to act on unless someone asks.

[thc1006]

Head is still 0c7004f8, the commit I checked on 25 September, so nothing has moved under either of us. What has moved is main: this branch is 12 commits behind it now, and the eight workflows here are all sitting in action_required, so nothing has built it since.

Merged origin/main into it locally. The merge is clean, no conflicts.

Worth saying why I checked that rather than reading the diff: git diff origin/main against a stale branch makes it look like the branch removes whatever main gained. On this one it appears to drop #4637's need_wait_more = true, revert #4452's CalculateNextRetryTime and next_retry_time_point_, and turn PerformCurlMessage back from bool to void. None of that is real. git merge-tree keeps all four, because your branch never touched those lines. I made the same misreading on one of my own pull requests earlier today, which is why I checked here before saying anything.

On the merged tree:

	result
build	clean
curl_http_test	38 of 38 pass

So the only thing in the way is the rebase and a workflow approval, neither of which is about your code.

Approving. Everything I left here before was an issue comment rather than a review, which does not count toward the two this repository needs, and that was an oversight on my part rather than a reservation about the change. The ordering in SendRequest is the one worth having: the early return lands before MaybeSpawnBackgroundThread(), so the background loop never starts against a null multi handle, and send_started_.exchange still happens exactly once per session after it.

[thc1006]

Correcting myself on the CI state, which is the part of this that matters, and re-stating the rest at today's main (cdd17eb1).

I wrote that the API reports 0 workflow runs for 0c7004f8. It reports 8: CI, clang-tidy, doxygen-tidy, include-what-you-use, CMake Install Tests, cppcheck, CodeQL and Zizmor, every one completed with conclusion: action_required, created on 25 September. My query passed a short sha, which that endpoint silently answers with 0. Check runs on that commit are genuinely 0, and the only status is EasyCLA, so nothing has executed. That is the first-contribution gate, and @Dyfintie cannot clear it.

I also wrote that earlier heads here did build, which reads as better news than the record supports. One head ever executed, 2c5efd26, and it went red: 31 of 75 check runs failed. Those 31 were a single missing global_log_handler.h include, diagnosed in this thread on 16 September and fixed in b6dd266f. Every head since has been re-gated, so no head carrying that fix has been built at all.

The rest, re-measured today. The head is still 0c7004f8. It is 49 commits behind main and merges cleanly: git merge-tree --write-tree origin/main 0c7004f8 reports no conflicts. The defect is still on main, at ext/src/http/client/curl/http_client_curl.cc:52, where curl_global_init(CURL_GLOBAL_ALL) is called and its result dropped; that is the only call site in the repo, and nothing reads a result anywhere.

@marcalff, could you release the workflow runs on this head when you have a moment? My approval stands and I re-verified the merge today, but it is not a required review, and CI has never built a head of this branch that carries the include fix.