Skip to content

[EXPORTER] Fix Elasticsearch log exporter Export blocking forever on a non-responding client - #4530

Open
om7057 wants to merge 4 commits into
open-telemetry:mainfrom
om7057:fix/elasticsearch-export-wait-deadline
Open

om7057 wants to merge 4 commits into
open-telemetry:mainfrom
om7057:fix/elasticsearch-export-wait-deadline

Conversation

@om7057

@om7057 om7057 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #4362.

The synchronous export path waited on its response condition variable with no deadline of its own, trusting the injected HttpClient to always eventually deliver a terminal event via OnResponse or OnEvent. Nothing in the HttpClient interface actually guarantees that: a client is a supported public surface, not a test seam, and one that accepts a request and never calls back (a dead thread, a reused socket, a swallowed error) left Export() blocked for the life of the process, with no way for a caller's Shutdown() to release it either.

Changes

  • waitForResponse() now takes an absolute std::chrono::steady_clock::time_point deadline instead of waiting unconditionally, via cv_.wait_until() in place of cv_.wait().
  • The deadline is derived from the exporter's own configured response_timeout_ and captured before SendRequest() is called, so it reflects this exporter's own timeout budget rather than whatever the client does with it.
  • A deadline that passes without a terminal event leaves the completion state at Pending, which reads as failure, the same outcome a terminal error event already produces today. No successful path changes.

Testing

Added SilentHttpClient/SilentSession test doubles whose SendRequest() never calls back into the handler at all (no OnResponse, no OnEvent), the exact scenario the issue describes. ExportReturnsOnTimeoutWhenClientNeverResponds constructs the exporter with a 1 second response_timeout_ and this client, and asserts Export() returns kFailure rather than hanging.

Verified the test actually catches the regression: reverted the fix locally (kept the test) and reran under a timeout wrapper, the test hung and was killed at exit code 124 instead of passing vacuously. Restored the fix and confirmed all four tests in the file pass, total runtime 1 second (the deadline in the new test), not 30 (the default response_timeout_).

@om7057
om7057 requested a review from a team as a code owner September 7, 2026 13:43
…a non-responding client

The synchronous export path waited on its response condition variable
with no deadline of its own, entirely trusting the injected HttpClient
to eventually deliver a terminal event via OnResponse or OnEvent.
Nothing in the HttpClient interface actually guarantees that: a client
that accepts a request and never calls back (a dead thread, a reused
socket, a swallowed error) left Export() blocked for the life of the
process, with no way for a caller's Shutdown() to release it either.

waitForResponse() now takes an absolute deadline, derived from the
exporter's own configured response timeout and captured before the
request is sent, so the wait is bounded independent of whether the
client honors its side of the contract. A deadline that passes without
a terminal event reads as failure, the same outcome a terminal error
event would already produce, so no successful path changes.

Added a SilentHttpClient/SilentSession test double whose SendRequest()
never calls back into its handler at all, and verified the regression
test actually catches the bug: reverting the fix locally makes the
test hang and get killed by its own timeout wrapper (exit 124), rather
than passing vacuously.

Fixes open-telemetry#4362
@om7057
om7057 force-pushed the fix/elasticsearch-export-wait-deadline branch from 83187ea to 37d54e4 Compare September 7, 2026 14:25
@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.51%. Comparing base (0d69d3e) to head (385ae3c).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4530   +/-   ##
=======================================
  Coverage   86.51%   86.51%           
=======================================
  Files         525      525           
  Lines       20469    20469           
=======================================
  Hits        17707    17707           
  Misses       2762     2762           
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.

@mateenali66 mateenali66 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this against the curl client with response_timeout = 1. A server that never replies fails the export at about 1s on both main and this PR, and replies at 990 to 999ms split the same way. So the change only matters for a custom client.

For that case, Export() still calls session->FinishSession() after the deadline (:495). SilentSession::FinishSession returns at once, so the test can't see it. A client built like curl waits there on the transfer (http_operation_curl.cc:523-538), so a dead worker thread still hangs Export(). curl's CancelSession() does not block (http_client_curl.cc:255-262). Should the expired path cancel instead?

The conflict is only CHANGELOG.md.

FinishSession() waits for the in-flight transfer to complete, which
defeats the purpose of the response deadline for HTTP clients (e.g.
curl) whose worker thread blocks on the transfer itself. Cancel the
session instead when the deadline expires.

Addresses review feedback from open-telemetry#4530 (review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Elasticsearch synchronous Export can block forever when an injected HTTP client never reports a terminal state

2 participants