[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.69%. Comparing base (ac7b87a) to head (05af53b).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4331   +/-   ##
=======================================
  Coverage   86.69%   86.69%           
=======================================
  Files         527      527           
  Lines       20902    20902           
=======================================
  Hits        18119    18119           
  Misses       2783     2783           

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.

[thc1006]

Correcting three figures I put on this pull request on 3 August, because they have drifted and the drift makes this change look larger than it is.

That comment said this one carries 23 lines of production code, #4297 211 and #4337 181. Measured again on the current heads: this one changes exporters/elasticsearch/src/es_log_record_exporter.cc by +6/-2, and four of those six lines are comment. #4297 is 263 lines across the new detail/es_bulk_response.h and the exporter, and #4337 is 188.

So the production change here is two calls. ReadError and WriteError log and break; every other terminal state in that switch already calls recordCompletion(CompletionState::Failure), eight of them. This adds the two that were missing.

A reviewer who would sooner spot check than read the whole thing has one question: do those two states really end a session with nothing further coming? If they do, the waiter is never released, because waitForResponse() is a cv_.wait on a predicate with no deadline of its own, and only recordCompletion reaches the notify. A synchronous export whose transfer fails that way does not return.

The test file is the rest of the diff, 265 lines to 590, and I would rather say why than leave it looking like padding. It adds a fake client that can deliver a terminal state after the handoff, which the existing fakes cannot, and eleven cases on top of the six already there. They cover each terminal state, and the orderings where a response and an I/O error arrive in either sequence, and a session destroyed while a request is still pending. If eleven is more than this change is worth, say so and I will cut it to ReadErrorEndsTheWait and WriteErrorEndsTheWait and keep the others for a follow-up. The number is not something I am attached to.

Two things that are not about the code. The failing check is the runner's security agent rather than the build: the failed step carries no compile or test output, only a 401 from the agent's own detection-rules API and a 500 from its upload step, and the same conan job is green on my other open pull requests. Rerunning it needs repository admin, so it will clear the next time main moves and I rebase.

And on #4502, @om7057 found that a comment of mine there named the wrong mechanism. isShutdown() returns before session_counter_.fetch_add(), so a refused export is never registered at all, and force_flush_m being a std::recursive_mutex is what keeps the re-entrant flush from deadlocking. The test was pinning correct behaviour and my explanation of why was carried over from the paragraph above it. It is corrected. Mentioning it here because it was found from reading one case, without any curl background, which is worth knowing if the size of these diffs is the obstacle.