Skip to content

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

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

[EXPORTER] Fix Elasticsearch log exporter Export blocking forever on a non-responding client#4530
om7057 wants to merge 2 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 83.46%. Comparing base (eec1b36) to head (dabaa7b).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4530   +/-   ##
=======================================
  Coverage   83.46%   83.46%           
=======================================
  Files         521      521           
  Lines       20412    20412           
=======================================
  Hits        17034    17034           
  Misses       3378     3378           
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.

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

1 participant