[BUG] Wait on a completion state in the Elasticsearch exporter

[thc1006]

Fixes #4296

The problem

bool waitForResponse()
{
  std::unique_lock<std::mutex> lk(mutex_);
  cv_.wait(lk);
  return response_received_;
}

OnResponse() set response_received_ under the mutex and called notify_all() after releasing it. If that landed before the exporting thread reached cv_.wait(lk), the notification was gone and the thread blocked with nothing left to wake it. The OnEvent() failure cases were worse: they called notify_all() without taking the mutex and without recording anything, so an early failure left the waiter with no state to observe even if it did wake.

A spurious wakeup also returned response_received_, still false, so a request that was still in flight was reported as a failure.

SessionState::Destroyed neither notified nor recorded anything, so a session that ended without a response or an error event left the waiter blocked indefinitely.

Two threads, one recording the outcome and one waiting, with the ordering flipped between runs:

=== notification arrives AFTER the waiter parks ===
  old: response after wait                 returned success
  new: response after wait                 returned success
=== notification arrives BEFORE the waiter parks ===
  new: response before wait                returned success
  old: response before wait                HUNG, no wakeup left to receive

The last run had to be killed by a timeout.

The fix

The handler records an explicit completion state under the mutex on both the success and the failure paths, and the wait is a predicate over that state:

cv_.wait(lk, [this] { return completion_ != CompletionState::Pending; });
return completion_ == CompletionState::Success;

Because the predicate is evaluated under the same mutex that publishes the state, an outcome recorded before the waiter arrives is simply observed rather than missed, and a spurious wakeup goes back to waiting instead of reporting a failure.

Recording is first writer wins. That matters for Destroyed, which now records a failure too: it fires at the end of every session, including successful ones, and must not overwrite a result that has already been recorded. Its purpose here is to release a waiter when a session ends without ever producing a response.

What this does not change

The wait still has no timeout of its own. It is bounded by the request timeout arriving as a TimedOut session event, plus the Destroyed fallback added here. Giving the wait its own deadline would be a behavior change beyond this fix, and I would rather do it separately if you want it.

Verification

• The comparison above was run, not reasoned about.
• clang-format 18 clean.
• I could not run the repo's clang-tidy against this file locally, since it needs the exporter's full include set. The enum carries an explicit std::uint8_t base type so it does not trip performance-enum-size, and <cstdint> is added for it. CI is the authority on warning_limit.

Scope

Synchronization only. The bulk response parsing is #4295 with its own PR, and the internal linkage of ResponseHandler belongs to the #4196 cleanup.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.30%. Comparing base (feea622) to head (5459227).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4298      +/-   ##
==========================================
+ Coverage   81.24%   81.30%   +0.06%     
==========================================
  Files         446      447       +1     
  Lines       18978    19028      +50     
==========================================
+ Hits        15417    15469      +52     
+ Misses       3561     3559       -2     

Files with missing lines	Coverage Δ
...orters/elasticsearch/src/es_log_record_exporter.cc	12.22% <ø> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[marcalff]

LGTM.

[marcalff]

@lalitb @ThomsonTan

Please review.

[lalitb]

This PR fixed the completion-state handling for most terminal events, but I think ReadError and WriteError still leave the state pending and may block Export() indefinitely. Can we both also call recordCompletion(CompletionState::Failure) for both the cases, like the other error states?

[lalitb]

Follow-up: could we add tests using an injected fake HTTP client? This should cover completion before the wait starts, ReadError, WriteError, Destroyed, and response followed by Destroyed. The current Elasticsearch exporter tests do not exercise this synchronization path.