[BUG] End the Elasticsearch exporter's wait on a read or write error

[thc1006]

Fixes #4330.

ResponseHandler::OnEvent records a completion for each session state that ends the session. ReadError and WriteError only log, so a client that reports either of them as its last event leaves the synchronous Export() waiting on its condition variable with nothing left to wake it. Two lines make those two behave like the other terminal states.

What this does not change, and one question for you

The bundled curl client dispatches nine session states and neither of these is among them, so this changes nothing for callers who use it. It matters for the HttpClient that the exporter's constructor accepts.

Worth putting in front of you rather than burying: otlp_http_client.cc handles ReadError and WriteError exactly the way the Elasticsearch exporter does today, logging at debug and carrying on. After this change the two exporters treat them differently. The SessionState comments say only "error reading response" and "error writing request", so there is no stated contract either way. If you would rather the two exporters stayed consistent, the alternative is to leave this alone and bound the wait instead, and I am happy to close this and open that one.

Evidence

Removing just the two recordCompletion() calls and rebuilding:

case	without the fix
ReadErrorEndsTheWait	never returns, killed at 15s
WriteErrorEndsTheWait	never returns, killed at 15s
AReadErrorAfterTheHandoffEndsTheExport	fails
IoErrorBeforeAResponseKeepsTheFailure	fails, kSuccess where kFailure is expected
ProgressStatesDoNotDecideTheResult, both SessionDestroyed cases	still pass

With the fix, the synchronous build runs 14 tests green and the async build skips the 11 that need the synchronous wait. The CTest timeout is there because a regression in this area stalls the job instead of failing an assertion.

What moved out

This pull request previously also made every terminal log line conditional on winning the completion race, and added an error line when a session is destroyed before a response arrives. That half needs a log-capturing fixture, it is a separate behaviour, and it is the reason the diff was four times this size. It is out, and I will open it separately once this one is settled. What is left is the two lines the title describes.

Verified locally: CMake in both the synchronous and async configurations with maintainer mode, Bazel, clang-format, cmake-format against the repository config, and markdownlint 0.46.0.

[codecov]

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4331   +/-   ##
=======================================
  Coverage   86.53%   86.53%           
=======================================
  Files         525      525           
  Lines       20482    20482           
=======================================
  Hits        17722    17722           
  Misses       2760     2760           

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

🚀 New features to boost your workflow:

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

[thc1006]

Context that saves working it out, since this one, #4297 and #4337 all sit on exporters/elasticsearch/src/es_log_record_exporter.cc and its test file.

Any two of the three conflict there, so whichever lands first the other two need a rebase. I will do those rebases, and there is no reason to take more than one of them.

Measured sizes, if it helps to pick a starting point. This one is 23 lines of production code and ends a wait that otherwise never returns. #4297 is 211 lines and stops a batch Elasticsearch rejected from reading as written. #4337 is 181 lines and the largest of the three. The test files are bigger than the production diffs in all three because each carries its own fake HTTP client; the second and third to land drop that copy when they rebase.

They fix separate things and none of them depends on the others, so the order is whatever suits you.