Skip to content

[BUG] Elasticsearch async exporter counts a session's completion zero or twice #4338

Description

@thc1006

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::SendAsync dispatches ConnectFailed and returns non-CURLE_OK when Setup() fails (ext/src/http/client/curl/http_operation_curl.cc, the code != CURLE_OK branch). Session::SendRequest then takes its else branch and dispatches CreateFailed for the same handler (ext/src/http/client/curl/http_client_curl.cc). This is deterministic, not a race.
  • The async completion lambda in Session::SendRequest uses two independent if blocks rather than else if, so an operation that was aborted and also has a response fires OnEvent(Cancelled) and OnResponse.

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIndicates an issue or PR lacks a `triage/foo` label and requires one.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions