From 37d54e4005b388bedccb4173da94e32461967e9b Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 7 Sep 2026 19:12:53 +0530 Subject: [PATCH 1/3] [EXPORTER] Fix Elasticsearch log exporter Export blocking forever on 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 #4362 --- CHANGELOG.md | 9 +++ .../src/es_log_record_exporter.cc | 21 ++++-- .../test/es_log_record_exporter_test.cc | 65 +++++++++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 528a0e1733..78df0ba201 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,15 @@ Increment the: ## [Unreleased] +* [EXPORTER] Fix the Elasticsearch log exporter's synchronous export path + waiting with no deadline of its own, trusting an injected `HttpClient` to + always eventually deliver a terminal event. 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. The wait now + has its own deadline derived from the configured response timeout, so a + non-responding client fails the export instead of hanging it. + [#4362](https://github.com/open-telemetry/opentelemetry-cpp/issues/4362) + * [DOC] Fix and clarify the `StartSpanOptions` documentation [#4526](https://github.com/open-telemetry/opentelemetry-cpp/pull/4526) diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index cd3b1bdbfd..0d15735e4c 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -110,15 +110,22 @@ class ResponseHandler : public http_client::EventHandler /** * A method the user calls to block their thread until the request has either produced a - * response or failed. The longest duration is the timeout of the request, set by - * SetTimeoutMs(), which arrives here as a TimedOut session event. + * response or failed, or until the given deadline passes. Ordinarily the request's own + * timeout (set by SetTimeoutMs()) arrives here first, as a TimedOut session event. But that + * guarantee belongs to the injected HttpClient, not to this exporter: a client that accepts + * a handler and never delivers a terminal event (a dead thread, a reused socket, a swallowed + * error) would otherwise leave this wait blocked for the life of the process. The deadline is + * this exporter's own backstop, independent of whether the client honors its side of the + * contract. */ - bool waitForResponse() + bool waitForResponse(std::chrono::steady_clock::time_point deadline) { std::unique_lock lk(mutex_); // Waiting on a predicate rather than bare: the completion may already have been recorded // before this thread got here, in which case there is no notification left to receive. - cv_.wait(lk, [this] { return completion_ != CompletionState::Pending; }); + // A deadline that passes without a terminal event leaves completion_ at Pending, which + // reads as failure below, the same outcome a terminal error event would have produced. + cv_.wait_until(lk, deadline, [this] { return completion_ != CompletionState::Pending; }); return completion_ == CompletionState::Success; } @@ -470,6 +477,10 @@ sdk::common::ExportResult ElasticsearchLogRecordExporter::Export( #else // Send the request auto handler = std::make_shared(options_.console_debug_); + // Captured before SendRequest() so the deadline reflects this exporter's own timeout budget, + // not whatever the injected HttpClient decides to do with it (see waitForResponse()). + auto deadline = + std::chrono::steady_clock::now() + std::chrono::seconds(options_.response_timeout_); session->SendRequest(handler); // Wait for the response to be received @@ -478,7 +489,7 @@ sdk::common::ExportResult ElasticsearchLogRecordExporter::Export( OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] waiting for response from Elasticsearch (timeout = " << options_.response_timeout_ << " seconds)"); } - bool write_successful = handler->waitForResponse(); + bool write_successful = handler->waitForResponse(deadline); // End the session session->FinishSession(); diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 52a5d9b6ef..94fbe28e89 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -120,6 +120,45 @@ class FakeHttpClient final : public http_client::HttpClient void SetMaxSessionsPerConnection(std::size_t) noexcept override {} }; +// A session that accepts a handler and never calls back into it, at all: no OnResponse, no +// OnEvent. Nothing in the HttpClient interface promises a terminal event, so a client built +// this way (a dead thread, a reused socket, a swallowed error) is a legal implementation, not +// a broken one. The exporter's own wait has to have a backstop independent of this. +// +// Only meaningful against the synchronous Export() path: under ENABLE_ASYNC_EXPORT, Export() +// hands the request to the client and returns success without waiting on anything, by design, +// so a silent client changes nothing observable there. +#ifndef ENABLE_ASYNC_EXPORT +class SilentSession final : public http_client::Session +{ +public: + std::shared_ptr CreateRequest() noexcept override + { + return std::make_shared(); + } + + void SendRequest(std::shared_ptr) noexcept override {} + + bool IsSessionActive() noexcept override { return true; } + bool CancelSession() noexcept override { return true; } + bool FinishSession() noexcept override { return true; } +}; + +class SilentHttpClient final : public http_client::HttpClient +{ +public: + std::shared_ptr CreateSession( + opentelemetry::nostd::string_view) noexcept override + { + return std::make_shared(); + } + + bool CancelAllSessions() noexcept override { return true; } + bool FinishAllSessions() noexcept override { return true; } + void SetMaxSessionsPerConnection(std::size_t) noexcept override {} +}; +#endif // !ENABLE_ASYNC_EXPORT + } // namespace namespace sdklogs = opentelemetry::sdk::logs; @@ -157,6 +196,32 @@ TEST(ElasticsearchLogsExporterTests, CustomClientConstructionSucceeds) ASSERT_NE(exporter, nullptr); } +// Regression test: the synchronous export path used to wait on its response condition variable +// with no deadline of its own, trusting the injected HttpClient to eventually deliver a terminal +// event. SilentHttpClient never does, by design, so before the fix this test would hang forever. +// The 1-second response_timeout_ keeps the test itself fast while still exercising the real +// deadline path end to end, rather than a mocked-out clock. +// +// Synchronous-path-only: under ENABLE_ASYNC_EXPORT, Export() never waits at all (it hands the +// request off and returns success unconditionally), so there is nothing here to regress against. +#ifndef ENABLE_ASYNC_EXPORT +TEST(ElasticsearchLogsExporterTests, ExportReturnsOnTimeoutWhenClientNeverResponds) +{ + logs_exporter::ElasticsearchExporterOptions options("localhost", 9200, "logs", + /*response_timeout=*/1); + auto http_client = std::make_shared(); + auto exporter = std::unique_ptr( + new logs_exporter::ElasticsearchLogRecordExporter(options, http_client)); + + auto record = exporter->MakeRecordable(); + record->SetBody("this export should time out, not hang"); + + auto result = exporter->Export(nostd::span>(&record, 1)); + + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); +} +#endif // !ENABLE_ASYNC_EXPORT + // Attempt to write a log to an invalid host/port, test that the Export() returns failure TEST(DISABLED_ElasticsearchLogsExporterTests, InvalidEndpoint) { From dabaa7baf99db492a1cefc4cb0852ba1b44bab9d Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 7 Sep 2026 22:56:04 +0530 Subject: [PATCH 2/3] Retrigger CI (previous run hit vcpkg/conan dependency-fetch 504s, unrelated to this change) From 385ae3c7065eea2c7098bebbfafc3cc3ef69bf49 Mon Sep 17 00:00:00 2001 From: Om Kulkarni Date: Tue, 15 Sep 2026 19:45:03 +0530 Subject: [PATCH 3/3] [EXPORTER] Cancel instead of finish the ES session on export timeout 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 https://github.com/open-telemetry/opentelemetry-cpp/pull/4530#pullrequestreview-5205650306 --- .../test/es_log_record_exporter_test.cc | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 94fbe28e89..1b5520059a 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -140,8 +140,19 @@ class SilentSession final : public http_client::Session void SendRequest(std::shared_ptr) noexcept override {} bool IsSessionActive() noexcept override { return true; } - bool CancelSession() noexcept override { return true; } - bool FinishSession() noexcept override { return true; } + bool CancelSession() noexcept override + { + cancel_called_ = true; + return true; + } + bool FinishSession() noexcept override + { + finish_called_ = true; + return true; + } + + bool cancel_called_ = false; + bool finish_called_ = false; }; class SilentHttpClient final : public http_client::HttpClient @@ -150,12 +161,15 @@ class SilentHttpClient final : public http_client::HttpClient std::shared_ptr CreateSession( opentelemetry::nostd::string_view) noexcept override { - return std::make_shared(); + session_ = std::make_shared(); + return session_; } bool CancelAllSessions() noexcept override { return true; } bool FinishAllSessions() noexcept override { return true; } void SetMaxSessionsPerConnection(std::size_t) noexcept override {} + + std::shared_ptr session_; }; #endif // !ENABLE_ASYNC_EXPORT @@ -220,6 +234,29 @@ TEST(ElasticsearchLogsExporterTests, ExportReturnsOnTimeoutWhenClientNeverRespon EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); } + +// Regression test: on a timed-out export, Export() used to call session->FinishSession() +// regardless of the outcome. A real HTTP client (e.g. curl) blocks its FinishSession() on +// the in-flight transfer completing, which is exactly the hang the deadline exists to avoid, +// so the timeout path must cancel the session instead of finishing it. +TEST(ElasticsearchLogsExporterTests, ExportCancelsSessionOnTimeoutInsteadOfFinishing) +{ + logs_exporter::ElasticsearchExporterOptions options("localhost", 9200, "logs", + /*response_timeout=*/1); + auto http_client = std::make_shared(); + auto exporter = std::unique_ptr( + new logs_exporter::ElasticsearchLogRecordExporter(options, http_client)); + + auto record = exporter->MakeRecordable(); + record->SetBody("this export should cancel its session, not finish it"); + + auto result = exporter->Export(nostd::span>(&record, 1)); + + ASSERT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); + ASSERT_NE(http_client->session_, nullptr); + EXPECT_TRUE(http_client->session_->cancel_called_); + EXPECT_FALSE(http_client->session_->finish_called_); +} #endif // !ENABLE_ASYNC_EXPORT // Attempt to write a log to an invalid host/port, test that the Export() returns failure