From 934f0d090febc7c56fbc183289a8c828cde44a34 Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 7 Sep 2026 19:12:53 +0530 Subject: [PATCH 1/6] [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 122cf6be0c..cdf8a29d09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,15 @@ Increment the: change. [#4624](https://github.com/open-telemetry/opentelemetry-cpp/pull/4624) +* [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) + * [EXAMPLES] Fix random attribute selection in metrics foo example to include all key-value pairs [#4585](https://github.com/open-telemetry/opentelemetry-cpp/pull/4585) 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 f82d8c991530be1ae18ea3fad8ee82ae3950b2a7 Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 7 Sep 2026 22:56:04 +0530 Subject: [PATCH 2/6] Retrigger CI (previous run hit vcpkg/conan dependency-fetch 504s, unrelated to this change) From 137e4eaa02e8b33b08cbbc7cf266ebde070ef8df Mon Sep 17 00:00:00 2001 From: Om Kulkarni Date: Tue, 15 Sep 2026 19:45:03 +0530 Subject: [PATCH 3/6] [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 From cdf582f58470e112f6b00b596da0140cfba50506 Mon Sep 17 00:00:00 2001 From: Om Kulkarni Date: Wed, 23 Sep 2026 20:38:09 +0530 Subject: [PATCH 4/6] Only cancel the session on an actual deadline expiry, not any failure The cancel-vs-finish branch was keyed on write_successful, so any terminal failure (ConnectFailed, SendFailed, CreateFailed) also took the CancelSession() path even though the transfer was already over by then. For curl, CancelSession() and FinishSession() are not interchangeable in that case. waitForResponse() now reports via an out-param whether the deadline passed with completion_ still Pending (a genuine still-outstanding transfer, which needs cancelling) as opposed to a terminal event having already arrived (which needs the normal FinishSession()). Added ExportFinishesSessionOnTerminalFailureInsteadOfCancelling with a FailFastSession/FailFastHttpClient pair that delivers ConnectFailed promptly, asserting FinishSession() is called and CancelSession() is not, and that the export returns well under the response_timeout_. Addresses review feedback from mateenali66. --- .../src/es_log_record_exporter.cc | 33 ++++++-- .../test/es_log_record_exporter_test.cc | 79 +++++++++++++++++++ 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index 0d15735e4c..b9a064c45c 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -117,8 +117,14 @@ class ResponseHandler : public http_client::EventHandler * 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. + * + * @param timed_out if not null, set to whether the deadline passed with nothing having + * reaped the transfer yet (completion_ still Pending), as opposed to a terminal event (a + * response, or a failure like ConnectFailed/SendFailed) having already arrived. The caller + * needs this distinction: only a still-outstanding transfer needs CancelSession() rather than + * FinishSession(), since a terminal event means the transfer is already over. */ - bool waitForResponse(std::chrono::steady_clock::time_point deadline) + bool waitForResponse(std::chrono::steady_clock::time_point deadline, bool *timed_out = nullptr) { std::unique_lock lk(mutex_); // Waiting on a predicate rather than bare: the completion may already have been recorded @@ -126,6 +132,10 @@ class ResponseHandler : public http_client::EventHandler // 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; }); + if (timed_out != nullptr) + { + *timed_out = (completion_ == CompletionState::Pending); + } return completion_ == CompletionState::Success; } @@ -489,10 +499,23 @@ 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(deadline); - - // End the session - session->FinishSession(); + bool timed_out = false; + bool write_successful = handler->waitForResponse(deadline, &timed_out); + + // Cancel only when the deadline genuinely expired with the transfer still outstanding: + // FinishSession() waits for an in-flight transfer to complete, which is exactly the hang + // this deadline exists to bound for HTTP clients (e.g. curl) whose worker thread blocks on + // the transfer itself. A terminal failure (ConnectFailed, SendFailed, CreateFailed, a + // response, ...) means the transfer is already over by the time waitForResponse returns, so + // FinishSession() is the correct call there, and for curl the two are not interchangeable. + if (timed_out) + { + session->CancelSession(); + } + else + { + session->FinishSession(); + } // If an error occurred with the HTTP request if (!write_successful) diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 1b5520059a..4fd47431eb 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -171,6 +171,56 @@ class SilentHttpClient final : public http_client::HttpClient std::shared_ptr session_; }; + +// A session that delivers a prompt terminal failure event instead of never responding, so a +// case can distinguish "the transfer is already over" from "the deadline expired with nothing +// having reaped it yet". ConnectFailed stands in for any of the terminal failure events +// (SendFailed, CreateFailed, ...) that OnEvent() records as CompletionState::Failure. +class FailFastSession final : public http_client::Session +{ +public: + std::shared_ptr CreateRequest() noexcept override + { + return std::make_shared(); + } + + void SendRequest(std::shared_ptr handler) noexcept override + { + handler->OnEvent(http_client::SessionState::ConnectFailed, ""); + } + + bool IsSessionActive() 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 FailFastHttpClient final : public http_client::HttpClient +{ +public: + std::shared_ptr CreateSession( + opentelemetry::nostd::string_view) noexcept override + { + 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 } // namespace @@ -257,6 +307,35 @@ TEST(ElasticsearchLogsExporterTests, ExportCancelsSessionOnTimeoutInsteadOfFinis EXPECT_TRUE(http_client->session_->cancel_called_); EXPECT_FALSE(http_client->session_->finish_called_); } + +// Regression test: the timeout-vs-finish branch used to key on whether the export succeeded, +// not on whether the deadline actually expired. A terminal failure event (ConnectFailed, +// SendFailed, CreateFailed, ...) also fails the export, but the transfer is already over by +// then, so it must still be handed back with FinishSession(), the same as a successful export; +// only a deadline that expires with nothing having reaped the transfer yet should cancel. +TEST(ElasticsearchLogsExporterTests, ExportFinishesSessionOnTerminalFailureInsteadOfCancelling) +{ + logs_exporter::ElasticsearchExporterOptions options("localhost", 9200, "logs", + /*response_timeout=*/30); + 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 fails fast and should finish its session, not cancel it"); + + auto start = std::chrono::steady_clock::now(); + auto result = exporter->Export(nostd::span>(&record, 1)); + auto elapsed = std::chrono::steady_clock::now() - start; + + ASSERT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); + ASSERT_NE(http_client->session_, nullptr); + EXPECT_TRUE(http_client->session_->finish_called_); + EXPECT_FALSE(http_client->session_->cancel_called_); + // Confirms the failure was reported promptly rather than by the 30s response_timeout_ + // expiring, which would also leave cancel_called_ true. + EXPECT_LT(elapsed, std::chrono::seconds(1)); +} #endif // !ENABLE_ASYNC_EXPORT // Attempt to write a log to an invalid host/port, test that the Export() returns failure From 9df0f35fa0f60169b3be63c86b06f2865d9f9ca3 Mon Sep 17 00:00:00 2001 From: Om Kulkarni Date: Sun, 18 Oct 2026 09:55:49 +0530 Subject: [PATCH 5/6] Fix response_timeout=0 mapping to a deadline already in the past waitForResponse()'s deadline was always derived as now() + seconds(response_timeout_), so response_timeout_ = 0 produced a deadline equal to now(), which the next instant is already behind. Every synchronous export then timed out immediately, even against a client that was about to answer successfully, since cv_::wait_until checks its predicate before checking the deadline only when the predicate is already satisfied at entry. response_timeout = 0 is documented as meaning no timeout, matching curl's own CURLOPT_TIMEOUT_MS 0 semantics, so a non-positive timeout now maps to steady_clock::time_point::max() instead. Added ZeroResponseTimeoutMeansNoTimeout with a DelayedSession that answers on a background thread after a genuine short delay, so the deadline comparison is actually exercised; confirmed it fails on the prior behavior and passes on this fix. Also corrected the waitForResponse() doc comment, which claimed a timeout surfaces as a TimedOut session event. It surfaces as SendFailed, matching the wording #4647 already uses for the same comment. Addresses review feedback from mateenali66. --- CHANGELOG.md | 6 +- .../src/es_log_record_exporter.cc | 21 ++++-- .../test/es_log_record_exporter_test.cc | 64 +++++++++++++++++++ 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2719c76987..91fbed46d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,7 +40,11 @@ Increment the: 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. + non-responding client fails the export instead of hanging it. A + `response_timeout` of 0 means no timeout, matching the curl client's own + `CURLOPT_TIMEOUT_MS` 0 semantics, so it no longer maps to a deadline + already in the past: that previously failed every synchronous export + immediately, even one the client was about to answer successfully. [#4362](https://github.com/open-telemetry/opentelemetry-cpp/issues/4362) * [METRICS SDK] Drop aggregation behaves as a no-op, prevent duplicate metric recording diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index b9a064c45c..a5e0e1e345 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -111,10 +111,12 @@ 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, 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 + * timeout (set by SetTimeoutMs()) arrives here first; the curl client reports that as a + * SendFailed session event, the same as any other transport failure, since it does not + * distinguish a timeout from other CURL errors. But a terminal event at all is a guarantee + * that belongs to the injected HttpClient, not to this exporter: a client that accepts a + * handler and never delivers one (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. * @@ -488,9 +490,14 @@ sdk::common::ExportResult ElasticsearchLogRecordExporter::Export( // 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_); + // not whatever the injected HttpClient decides to do with it (see waitForResponse()). A + // non-positive response_timeout_ means no timeout, matching the curl client's own + // CURLOPT_TIMEOUT_MS 0 semantics, so it gets the max time_point rather than one already in + // the past; this backstop only bounds the wait when the caller actually asked for a bound. + auto deadline = options_.response_timeout_ > 0 + ? std::chrono::steady_clock::now() + + std::chrono::seconds(options_.response_timeout_) + : std::chrono::steady_clock::time_point::max(); session->SendRequest(handler); // Wait for the response to be received diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 4fd47431eb..188a3af92d 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include "nlohmann/json.hpp" @@ -120,6 +121,47 @@ class FakeHttpClient final : public http_client::HttpClient void SetMaxSessionsPerConnection(std::size_t) noexcept override {} }; +// A session that answers on a background thread after a short, fixed delay, so the exporter's +// wait genuinely blocks for a moment rather than finding the response already recorded: a +// response_timeout_ of 0 that mapped to a deadline already in the past would still pass a wait +// whose predicate is satisfied before the wait is even entered, so this is the only way to +// exercise a deadline comparison at all. +class DelayedSession final : public http_client::Session +{ +public: + std::shared_ptr CreateRequest() noexcept override + { + return std::make_shared(); + } + + void SendRequest(std::shared_ptr handler) noexcept override + { + std::thread([handler] { + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + FakeResponse response; + handler->OnResponse(response); + }).detach(); + } + + bool IsSessionActive() noexcept override { return true; } + bool CancelSession() noexcept override { return true; } + bool FinishSession() noexcept override { return true; } +}; + +class DelayedHttpClient 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 {} +}; + // 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 @@ -336,6 +378,28 @@ TEST(ElasticsearchLogsExporterTests, ExportFinishesSessionOnTerminalFailureInste // expiring, which would also leave cancel_called_ true. EXPECT_LT(elapsed, std::chrono::seconds(1)); } + +// Regression test: response_timeout = 0 is documented to mean no timeout, matching curl's own +// CURLOPT_TIMEOUT_MS 0 semantics. Deriving the wait's deadline as now() + seconds(0) instead +// mapped that to a deadline already in the past, so every sync export failed immediately even +// when the client was about to answer successfully. DelayedSession answers on a separate +// thread after a short, genuine delay, so a deadline already elapsed and a deadline far in the +// future are actually distinguishable here. +TEST(ElasticsearchLogsExporterTests, ZeroResponseTimeoutMeansNoTimeout) +{ + logs_exporter::ElasticsearchExporterOptions options("localhost", 9200, "logs", + /*response_timeout=*/0); + 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 wait for the delayed response, not time out instantly"); + + auto result = exporter->Export(nostd::span>(&record, 1)); + + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); +} #endif // !ENABLE_ASYNC_EXPORT // Attempt to write a log to an invalid host/port, test that the Export() returns failure From 4608ec21a949c81c6c02249ebcabc72c6c195516 Mon Sep 17 00:00:00 2001 From: Om Kulkarni Date: Tue, 20 Oct 2026 18:39:41 +0530 Subject: [PATCH 6/6] Fix clang-format wrapping on the response_timeout deadline ternary --- exporters/elasticsearch/src/es_log_record_exporter.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index a5e0e1e345..48560b35d9 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -494,10 +494,10 @@ sdk::common::ExportResult ElasticsearchLogRecordExporter::Export( // non-positive response_timeout_ means no timeout, matching the curl client's own // CURLOPT_TIMEOUT_MS 0 semantics, so it gets the max time_point rather than one already in // the past; this backstop only bounds the wait when the caller actually asked for a bound. - auto deadline = options_.response_timeout_ > 0 - ? std::chrono::steady_clock::now() + - std::chrono::seconds(options_.response_timeout_) - : std::chrono::steady_clock::time_point::max(); + auto deadline = + options_.response_timeout_ > 0 + ? std::chrono::steady_clock::now() + std::chrono::seconds(options_.response_timeout_) + : std::chrono::steady_clock::time_point::max(); session->SendRequest(handler); // Wait for the response to be received