Repository navigation
[BUG] Elasticsearch async exporter counts a session's completion zero or twice #4338
Description
Describe your environment
main at 11fa0db0, Linux, GCC 14.2, -DWITH_ELASTICSEARCH=ON -DWITH_ASYNC_EXPORT_PREVIEW=ON. AsyncResponseHandler only exists under ENABLE_ASYNC_EXPORT.
Steps to reproduce
Two exports through a fake HttpClient injected via the public constructor. The first session reports two terminal events, which is what the curl path does on a setup failure (see below). The second never calls back.
int session = 0;
auto client = std::make_shared<FakeHttpClient>([&session](http_client::EventHandler &handler) {
if (++session == 1)
{
handler.OnEvent(http_client::SessionState::ConnectFailed, "");
handler.OnEvent(http_client::SessionState::CreateFailed, "");
}
// session 2 never calls back
});
ExportOnce(exporter); // session 1
ExportOnce(exporter); // session 2, still in flight
const bool flushed = exporter.ForceFlush(std::chrono::milliseconds{20});What is the expected behavior?
flushed == false. One of the two sessions has not finished.
What is the actual behavior?
[repro] ForceFlush with one session still running returned true
Immediately, in 0 ms. finished_session_counter_ reached 2 from a single session, so finished >= running was already true.
I ran this on top of the fix in #4337, so this is not the timeout defect reported in #4336. The counters are monotonic and compared with >=, which makes the overshoot permanent: every later flush also returns one session early.
Additional context
AsyncResponseHandler accounts a session's completion neither at most once nor at least once.
More than once. OnResponse and every handled terminal OnEvent call result_callback_ directly, and the callback increments finished_session_counter_. There is no guard. Two in-tree paths produce two terminal callbacks for one session:
HttpOperation::SendAsyncdispatchesConnectFailedand returns non-CURLE_OKwhenSetup()fails (ext/src/http/client/curl/http_operation_curl.cc, thecode != CURLE_OKbranch).Session::SendRequestthen takes itselsebranch and dispatchesCreateFailedfor the same handler (ext/src/http/client/curl/http_client_curl.cc). This is deterministic, not a race.- The async completion lambda in
Session::SendRequestuses two independentifblocks rather thanelse if, so an operation that was aborted and also has a response firesOnEvent(Cancelled)andOnResponse.
Fewer than once. ReadError, WriteError and Destroyed fall into default: break, so result_callback_ never fires and the session is never counted as finished. The synchronous handler treats all three as terminal, Destroyed since #4298 and the other two in #4331, so the two handlers in the same file currently disagree.
Suggested shape
Route every outcome through one idempotent step:
void CompleteOnce(sdk::common::ExportResult result) noexcept
{
bool expected = false;
if (completed_.compare_exchange_strong(expected, true, std::memory_order_acq_rel))
{
result_callback_(result);
}
}with OnResponse and every terminal OnEvent going through it, and the switch enumerating all fifteen states so -Wswitch catches a future addition rather than a default: swallowing it. The guard is also what makes widening the terminal set safe, since it removes the double-completion objection to treating ReadError, WriteError and Destroyed as terminal here the way the synchronous handler does.
Worth covering: each of the three ignored states finishing one session as a failure, a response followed by another terminal event counting once, a cancellation followed by a late response counting once, and a duplicate terminal event counting once.
Two things I would rather flag than fold in. The two if blocks and the non-returning gzip failure branch in Session::SendRequest are shared HTTP client behaviour, so fixing them there would remove the double-fire for every consumer, but that is a separate change. And an exporter side guard is still warranted either way, because EventHandler is a public extension point and an injected client is free to emit whatever it likes.
I am happy to open the PR for the exporter side. It overlaps #4297 and #4337 in the same file, so I would rather land those first unless you would prefer it sooner.
Activity
Count an async Elasticsearch export exactly once
Count an async Elasticsearch export exactly once
Count an async Elasticsearch export exactly once
Count an async Elasticsearch export exactly once
Count an async Elasticsearch export exactly once
Count an async Elasticsearch export exactly once
Count an async Elasticsearch export exactly once
github-actions commentedon Oct 4, 2026
This issue was marked as stale due to lack of activity.
thc1006 commentedon Oct 24, 2026
Still live on
mainatac7b87a8, and it now sits in front of a pull request that is waiting on review, so it is worth taking the stale label off.The reproduction in the description still holds. What I did not have in August is what the duplicate costs once
ForceFlushstarts trusting the counter. #4523 changes the return fromtimeout_steady > 0tofinished_session_counter_ >= running_counter, which is the right question, and the over-count answers it with the wrong export. Two exports, the first deliveringConnectFailedthenCreateFailed, the second accepted and never answered, three runs per cell, correct answer false everywhere:mainac7b87a8The control row is what isolates this issue: with a single terminal event #4523 waits out the deadline and answers honestly, so the duplicate is the only difference between those two cells. #4502 on its own closes the duplicate, which is why its two cells agree, and it still reports true because it carries
main's return.The production halves merge without conflict; only the changelog and the test file collide.