[om7057]

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_).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.81%. Comparing base (a273994) to head (4608ec2).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4530      +/-   ##
==========================================
- Coverage   86.81%   86.81%   -0.00%     
==========================================
  Files         525      525              
  Lines       20739    20736       -3     
==========================================
- Hits        18003    18000       -3     
  Misses       2736     2736              

Files with missing lines	Coverage Δ
...orters/elasticsearch/src/es_log_record_exporter.cc	47.73% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mateenali66]

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.

[om7057]

Good catch, and sorry for the delay replying. This was actually fixed same-day in 385ae3c7: the expired path now calls session->CancelSession() instead of session->FinishSession(), since FinishSession() waiting on the in-flight transfer is exactly the hang the deadline exists to bound for a client like curl.

Added ExportCancelsSessionOnTimeoutInsteadOfFinishing, which asserts CancelSession() is called and FinishSession() is not on a timed-out export. Should be covered now; let me know if there's a gap I'm missing.

[om7057]

cc: @dbarker @marcalff could you please check this one?

[mateenali66]

No gap in the timeout path, that one holds. Built the head and swapped the silent session for one that delivers a prompt ConnectFailed. It cancels too: 0 ms elapsed, cancel called, finish not, with response_timeout at 30.

The branch is on write_successful, not on deadline expiry, so ConnectFailed, SendFailed and CreateFailed take it as well. Those record Failure promptly with the transfer already over, and for curl the two calls differ. FinishSession waits on the result future, CancelSession aborts and does not. So three paths stop reaping the transfer, which is more than the comment above the branch claims.

Pending is already the signal. A deadline expiry leaves completion_ there, every other failure sets Failure. Worth having waitForResponse report which of the two it left on?

[om7057]

Confirmed, the branch was keyed on write_successful rather than on the deadline itself. Fixed in `295ec739`: waitForResponse() now reports via an out-param whether completion_ was still Pending when the deadline passed, and only that case cancels; a terminal failure (ConnectFailed, SendFailed, CreateFailed) finishes the session normally since the transfer is already over.

Added ExportFinishesSessionOnTerminalFailureInsteadOfCancelling with a FailFastSession that delivers ConnectFailed immediately, asserting FinishSession() runs and CancelSession() does not, and that the export returns well under the 30s response_timeout_.

On the waitForResponse() reporting which state it left on: went with an out-param (timed_out) rather than exposing CompletionState directly, so the caller only sees the one bit it needs. Let me know if you'd rather have the full state exposed for something else.

[thc1006]

Checked cdf582f5 here. 75 checks green on this one, so unlike some of its neighbours CI has actually exercised it; what follows is the part CI does not tell you.

I filed #4362, and the shape I suggested there was wrong. It was only the deadline. That on its own would have moved the hang rather than removed it, because Export() calls FinishSession() next, and in http_operation_curl.cc HttpOperation::Finish() does this:

if (async_data_ && async_data_->result_future.valid())
{
  if (HttpOperationAccessor::GetThreadId(*async_data_) != std::this_thread::get_id())
  {
    static_cast<void>(async_data_->result_future.get());
  }
}

Export() is not the IO thread, so that get() blocks until the promise is set, and the promise is set from Cleanup() on the thread that never came back. The wait would have returned on time and then stopped in the same place. Abort() by contrast only raises is_aborted_ and schedules the removal. So the timed_out flag and the CancelSession() branch are the part that actually closes #4362, and the comment you put above them says exactly why. That is a correction to my issue, not to your change.

Ran the three new cases both ways, gcc 14, clean worktree:

	result
the three cases on this head	pass, 3 runs of 3
the whole Elasticsearch suite on this head	6 of 6
the three cases with only main's es_log_record_exporter.cc swapped back	ExportReturnsOnTimeoutWhenClientNeverResponds starts and never returns, killed at 120 s

That last row is the issue: not a wrong answer, a thread that does not come back. Your case pins it rather than describing it.

One coordination note. This and my #4331 both edit waitForResponse, you the signature and me the two missing recordCompletion calls on ReadError and WriteError. git merge-tree reports a content conflict between the two branches, so whichever lands second needs a rebase. Yours is older and green; mine is the one that should move, and I will do that rebase rather than leave it to you.

Approving. The deadline is captured before SendRequest() where it should be, the Pending state reads as failure the same way a terminal error would, and the cancel is on the one path that needs it.

[om7057]

@marcalff could you please check this PR once? It already has an approval from thc1006.

[mateenali66]

re-checked at d52dc711, both earlier points are fixed. ExportFinishesSessionOnTerminalFailureInsteadOfCancelling fails against the e2a08111 source and passes now, and the ES suite is clean under tsan and asan.

one behaviour change. response_timeout = 0 now fails every sync export. the deadline at es_log_record_exporter.cc:492 has already passed, so it cancels in 0 ms even when the server answers at once. on main 0 reaches curl as CURLOPT_TIMEOUT_MS 0, which means no timeout, so exports succeed. if 0 should keep working, applying the deadline only when the timeout is positive would do it.

also the comment at :113 says curl's own timeout normally arrives as TimedOut. curl reports it as SendFailed (#4604), and #4647 rewrites the same block, so the two PRs will conflict there.

[om7057]

Confirmed, response_timeout = 0 mapped to now() as the deadline, which is already behind the instant waitForResponse() checks it. Fixed by mapping a non-positive timeout to time_point::max() instead, matching curl's own CURLOPT_TIMEOUT_MS 0 semantics. Added ZeroResponseTimeoutMeansNoTimeout with a session that answers after a genuine short delay on a background thread, since a synchronous fake session's predicate would already be satisfied before the deadline is even checked. Confirmed it fails on the prior code and passes on the fix.

Also updated the waitForResponse() comment to say SendFailed instead of TimedOut, matching the wording #4647 already uses for the same block. Good catch on the two PRs touching the same comment, whichever merges first the other will need a small rebase there.

[om7057]

@mateenali66 pushed the fix for the response_timeout=0 case and the comment correction, could you take another look when you get a chance?

[mateenali66]

re-checked at 5c6e465b. response_timeout = 0 works again with the curl client against a local server, and ZeroResponseTimeoutMeansNoTimeout fails on the old source and passes now. the comment fix is in too.

one catch with time_point::max(). without pthread_cond_clockwait, libstdc++'s wait_until converts the steady deadline to system_clock, that addition overflows for max(), and the predicate loop never exits. on alpine 3.20 (musl, g++ 13.2.1) ZeroResponseTimeoutMeansNoTimeout hangs and the other 6 pass. gcc 9 on glibc 2.31 hangs the same way in a small repro, and docker/build.sh defaults to alpine. a plain cv_.wait(lk, pred) when the timeout isn't positive avoids it.

Format also wants a clang-format pass on es_log_record_exporter.cc.

[thc1006]

Retraction. An earlier version of this comment claimed the timeout path can call a destroyed handler. I built the branch and it cannot. Sorry for the noise.

What I missed: Session::SendRequest hands HttpOperation::SendAsync a lambda that captures the shared_ptr<EventHandler> by value, and SendAsync stores it at http_operation_curl.cc:1503 in AsyncData::callback, a std::function member. I had grepped for the type name, which cannot see a capture written as callback.

The ordering inside Cleanup() is what makes it safe, and it is worth naming because it is not obvious: the Cancelled dispatch is at http_operation_curl.cc:553 and the captured pointer is only released at :578, by a swap into a local that dies when Cleanup() returns. So the handler is still owned at the moment of every dispatch, and a session parked for abort cannot outlive it.

Measured on 5c6e465b: the exporter built from options alone so it uses the real curl client, AddressSanitizer, response_timeout_ of 1 second, against a server that accepts the connection and never answers.

	result
Export()	returns kFailure at 1000 ms, 5 runs of 5
OnEvent calls arriving after it returns	2, 5 runs of 5
AddressSanitizer	silent, 5 runs of 5
control, server answers	kSuccess in 4 ms, nothing arrives late, ASan silent
control, deliberate use-after-free of the same shape	reported, so ASan was armed

One thing did come out of it. On the timeout path the handler takes two terminal events after Export() has returned, one from HttpOperation::Cleanup and one from that completion lambda. Harmless here, since the handler is alive and nothing reads it again. It is item 4 of #4360 and not something this introduces, but it is worth knowing that this path reaches it: main's synchronous path always took FinishSession, which blocks in HttpOperation::Finish on result_future.get() until the transfer is over, so the double dispatch always landed before Export() returned.

The two things I checked that do hold. time_point::max() for a non-positive response_timeout_ keeps "no timeout" meaning no timeout, where now() + seconds(0) would fail every such export at once. And CancelSession() on the timeout path is the right call, since FinishSession() blocks on the transfer, which is the hang the deadline exists to bound.

Merge order: no conflicts between this and my #4297, #4337 or #4502. #4331 gives one conflict, in es_log_record_exporter.cc, and that one is mine to resolve.