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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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