[BUG] Prevent lost condition-variable wakeups during OTLP file, periodic metric, and batch span processor shutdown by mike-nugent-mongodb · Pull Request #4365 · open-telemetry/opentelemetry-cpp

mike-nugent-mongodb · 2026-08-05T19:12:04Z

Changes

The issue in each changed spot is that there is a race between the "shutdown" call and the worker thread, such that the worker thread might not actually shut down. The worker will eventually shut down on the next configured periodic export, but if this could be configured to be relatively long (e.g. 1 minute). There's no workaround for this:

ForceFlush will early-return after shutdown, so one can't use ForceFlush to un-stuck a worker thread.
Calling Shutdown more than once can lead to undefined behavior if it tries to join the worker thread from two different threads - at the very least, the return value of joinable is undefined, so a second thread trying to check if the worker is still joinable might see joinable as false if the thread has been joined but is stuck.

I did not update CHANGELOG.md since the change seems small, but let me know if I should. I did not add unit tests as they would need to be invasive and complicated to reliably catch these. AFAIK there is no existing bug/issue documenting this.

…dic metric, and batch span processor shutdown.

linux-foundation-easycla · 2026-08-05T19:12:11Z

The committers listed above are authorized under a signed CLA.

✅ login: mike-nugent-mongodb / name: Mike Nugent (e35bb01)

codecov · 2026-08-08T16:15:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.64%. Comparing base (1d54d33) to head (18f3bd4).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4365      +/-   ##
==========================================
+ Coverage   82.63%   82.64%   +0.02%     
==========================================
  Files         512      512              
  Lines       20138    20150      +12     
==========================================
+ Hits        16639    16651      +12     
  Misses       3499     3499

Files with missing lines	Coverage Δ
exporters/otlp/src/otlp_file_client.cc	`70.72% <100.00%> (+0.35%)`	⬆️
...metrics/export/periodic_exporting_metric_reader.cc	`74.14% <100.00%> (+0.93%)`	⬆️

🚀 New features to boost your workflow:

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

mateenali66

reader and file exporter fixes look right. per the overlap discussion on #4382: dropping the batch_span_processor.cc hunk here would let both PRs land without conflict, the other two fixes aren't covered there at all. also needs a CHANGELOG entry

mateenali66 · 2026-08-10T00:31:54Z

+    {
+      // Acquiring cv_m_ guarantees that the next time the worker thread checks the wait condition
+      // on cv_ (either from notify below or any other reason) it will see IsShutdown() return true.
+      std::lock_guard<std::mutex> cv_guard{cv_m_};


fence is right, the wait predicate checks IsShutdown() under cv_m_. the force flush path below has the same store-then-notify shape (the "must not wait for ever" workaround), in scope here or follow-up?

Since it's in the same vein, I've added the change to ForceFlush as well.

ForceFlush change looks right, and no lock-order problem with it: this path takes force_flush_m_ then cv_m_ inside the predicate, and the worker's cv_m_ lock is scoped to the do-while body so it is released before CollectAndExportOnce runs.

One direction is still open though. This fixes the waker side (cv_). The completion side is unchanged: CollectAndExportOnce CASes force_flush_notified_sequence_ and calls force_flush_cv_.notify_all() holding nothing, so an OnForceFlush caller can evaluate the predicate, miss the notify, then wait. That is what the "must not wait for ever" chunked wait is compensating for, so the chunked loop has to stay as long as that store-then-notify is unguarded. Same bucket as the wait_for race you described, fine as a follow-up, just noting why that workaround can't come out yet.

mateenali66 · 2026-08-10T00:32:23Z

+            // Even though is_shutdown is atomic, the lock guarantees that either a change to
+            // is_shutdown will be observed, or background_thread_waker_cv will see the notification
+            // at shutdown.
+            if (concurrency_file->is_shutdown.load(std::memory_order_acquire))


with the early check gone, shutdown is observed only after one more flush pass, intended? fine if it's deliberate flush-on-shutdown, just checking

The check on is_shutdown occurs at almost the same time as previously, the only difference is that the check now occurs while holding concurrency_file->background_thread_waker_lock instead of prior to acquiring it. In both cases, if a flush is occurring while is_shutdown is set then the code will break prior to the next wait_for. Does that make sense or am I misunderstanding?

While looking into this I do notice there is an issue where a ForceFlush may not cause an additional flush and make take the full flush interval if file_->background_thread_waker_cv.notify_all(); happens just before the background thread blocks on concurrency_file->background_thread_waker_cv.wait_for(lk, flush_interval);. Fixing this would require a more involved change, so I'd prefer that to be separate. Some details on that: if you just add a variable to cause background_thread_waker_cv.wait_for to exit when it's set (e.g. is_force_background_thread_wake), the problem is that ForceFlush waits on background_thread_waiter_cv , so it could miss the notification from the background thread unless there is some additional synchronization (e.g., acquire the waiter lock before releasing the waker lock).

Re-read the loop against the head commit, the only thing between the old check position and the new one is the BeforeWait() hook, so no extra flush pass. Agreed.

That hook is the one thing the move does change. On the shutdown path BeforeWait() now fires and AfterWait() never does, because the break is inside the lock scope. Before this PR the shutdown break came ahead of BeforeWait(), so the pair stayed balanced. thread_instrumentation.h documents the two as bracketing a blocking wait, so an app that flips thread state in BeforeWait and restores it in AfterWait leaves it set at thread exit. Hoisting the break out of the lock scope keeps both the fence and the pairing:

bool shutdown_requested = false; { std::unique_lock<std::mutex> lk(concurrency_file->background_thread_waker_lock); shutdown_requested = concurrency_file->is_shutdown.load(std::memory_order_acquire); if (!shutdown_requested) { concurrency_file->background_thread_waker_cv.wait_for(lk, flush_interval); } }

then break after the AfterWait() block. Agreed the ForceFlush / wait_for race is a separate change.

std::unique_lock<std::mutex> lk(concurrency_file->background_thread_waker_lock); concurrency_file->background_thread_waker_cv.wait_for(lk, flush_interval, [concurrency_file]() { return concurrency_file->is_shutdown.load(std::memory_order_acquire); });

Is these codes more clear? We also use conditional wait_for in other components.

Good catch, I've moved the break to after AfterWait.

As far as using the conditional wait_for, I don't think it would be a lot clearer right now because is_shutdown has to kept outside the function, so the code would be more like

std::unique_lock<std::mutex> lk(concurrency_file->background_thread_waker_lock); concurrency_file->background_thread_waker_cv.wait_for(lk, flush_interval, [concurrency_file, &is_shutdown]() { return is_shutdown = concurrency_file->is_shutdown.load(std::memory_order_acquire); });

but let me know if this is preferred.

lalitb · 2026-08-10T21:25:17Z

    {
+      {
+        std::lock_guard<std::mutex> waker_guard{file_->background_thread_waker_lock};
+        file_->is_shutdown.store(true, std::memory_order_release);


Could we apply this same guarded store plus notify in OtlpFileSystemBackend::Shutdown() too? When there is nothing pending, ForceFlush() returns before notifying the worker, so Shutdown() can return while the background thread remains parked until flush_interval. The destructor now handles this correctly, but callers that keep the client alive after Shutdown() still retain the delayed worker.

lalitb · 2026-08-10T21:27:04Z

  if (worker_thread_.joinable())
  {
-    synchronization_data_->is_force_wakeup_background_worker.store(true, std::memory_order_release);
+    {


Could we drop the BatchSpanProcessor changes from this PR? #4382 contains the more complete fix for this path, including ForceFlush, completion notification, and regression coverage. Keeping the trace fix there avoids overlapping changes between the two PRs.

Done, I've removed BatchSpanProcessor changes.

mike-nugent-mongodb

I've removed the BatchSpanProcessor and added the change to CHANGELOG in addition to addressing the comments.

mike-nugent-mongodb · 2026-08-19T16:27:09Z

  if (worker_thread_.joinable())
  {
-    synchronization_data_->is_force_wakeup_background_worker.store(true, std::memory_order_release);
+    {


Done, I've removed BatchSpanProcessor changes.

mike-nugent-mongodb · 2026-08-19T16:32:23Z

    {
+      {
+        std::lock_guard<std::mutex> waker_guard{file_->background_thread_waker_lock};
+        file_->is_shutdown.store(true, std::memory_order_release);


mike-nugent-mongodb · 2026-08-19T16:51:55Z

+    {
+      // Acquiring cv_m_ guarantees that the next time the worker thread checks the wait condition
+      // on cv_ (either from notify below or any other reason) it will see IsShutdown() return true.
+      std::lock_guard<std::mutex> cv_guard{cv_m_};


Since it's in the same vein, I've added the change to ForceFlush as well.

mike-nugent-mongodb · 2026-08-19T19:12:21Z

+            // Even though is_shutdown is atomic, the lock guarantees that either a change to
+            // is_shutdown will be observed, or background_thread_waker_cv will see the notification
+            // at shutdown.
+            if (concurrency_file->is_shutdown.load(std::memory_order_acquire))


The check on is_shutdown occurs at almost the same time as previously, the only difference is that the check now occurs while holding concurrency_file->background_thread_waker_lock instead of prior to acquiring it. In both cases, if a flush is occurring while is_shutdown is set then the code will break prior to the next wait_for. Does that make sense or am I misunderstanding?

While looking into this I do notice there is an issue where a ForceFlush may not cause an additional flush and make take the full flush interval if file_->background_thread_waker_cv.notify_all(); happens just before the background thread blocks on concurrency_file->background_thread_waker_cv.wait_for(lk, flush_interval);. Fixing this would require a more involved change, so I'd prefer that to be separate. Some details on that: if you just add a variable to cause background_thread_waker_cv.wait_for to exit when it's set (e.g. is_force_background_thread_wake), the problem is that ForceFlush waits on background_thread_waiter_cv , so it could miss the notification from the background thread unless there is some additional synchronization (e.g., acquire the waiter lock before releasing the waker lock).

mateenali66 · 2026-08-20T05:21:49Z

branch is conflicting against main now that #4382 has landed, needs a rebase before CI can give a clean signal here

owent · 2026-08-20T06:43:53Z

+            if (concurrency_file->is_shutdown.load(std::memory_order_acquire))
+            {
+              break;
+            }


In my understanding, When concurrency_file->is_shutdown is true, std::fflush should still need to be called one more time.So Shutdown will still flush all pending records.

I think this is a preexisting issue, as previously is_shutdown did not cause an additional fflush, so I'd prefer to leave off fixing this.

In the existing codes. Shutdown() will calls ForceFlush onece, which will wake up the background thread and calls std::fflush.

mike-nugent-mongodb

branch is conflicting against main now that #4382 has landed, needs a rebase before CI can give a clean signal here

I have added merge commits with main, so #4382 changes are included. The only conflict was from CHANGELOG.

mike-nugent-mongodb · 2026-08-20T13:00:26Z

+            if (concurrency_file->is_shutdown.load(std::memory_order_acquire))
+            {
+              break;
+            }


I think this is a preexisting issue, as previously is_shutdown did not cause an additional fflush, so I'd prefer to leave off fixing this.

mike-nugent-mongodb · 2026-08-20T13:10:26Z

+            // Even though is_shutdown is atomic, the lock guarantees that either a change to
+            // is_shutdown will be observed, or background_thread_waker_cv will see the notification
+            // at shutdown.
+            if (concurrency_file->is_shutdown.load(std::memory_order_acquire))


Good catch, I've moved the break to after AfterWait.

As far as using the conditional wait_for, I don't think it would be a lot clearer right now because is_shutdown has to kept outside the function, so the code would be more like

std::unique_lock<std::mutex> lk(concurrency_file->background_thread_waker_lock); concurrency_file->background_thread_waker_cv.wait_for(lk, flush_interval, [concurrency_file, &is_shutdown]() { return is_shutdown = concurrency_file->is_shutdown.load(std::memory_order_acquire); });

but let me know if this is preferred.

[BUG] Prevent lost condition-variable wakeups during OTLP file, perio…

e35bb01

…dic metric, and batch span processor shutdown.

mike-nugent-mongodb requested a review from a team as a code owner August 5, 2026 19:12

denizariyan mentioned this pull request Aug 8, 2026

[SDK] Fix lost wakeup in BatchSpanProcessor shutdown/force-flush notify #4382

Merged

3 tasks

mateenali66 reviewed Aug 10, 2026

View reviewed changes

lalitb reviewed Aug 10, 2026

View reviewed changes

denizariyan mentioned this pull request Aug 10, 2026

[BUG] BatchLogRecordProcessor: lost condvar wakeup can stall Shutdown/ForceFlush by up to one schedule_delay #4400

Open

mateenali66 mentioned this pull request Aug 14, 2026

REQUEST: New membership for mateenali66 open-telemetry/community#3640

Closed

6 tasks

owent added a commit to owent/opentelemetry-cpp-contrib that referenced this pull request Aug 17, 2026

Merge concunrrency fixes from <open-telemetry/opentelemetry-cpp#4365>

ba10b0e

mike-nugent-mongodb added 4 commits August 19, 2026 16:29

Revert changes to batch_span_processor.cc

6551f60

Update changelog and fix additional races

b28f264

Merge remote-tracking branch 'fork/main' into exporter-shutdown-race

255f810

Add PR to changelog

c2e05e4

mike-nugent-mongodb commented Aug 19, 2026

View reviewed changes

mike-nugent-mongodb requested review from lalitb and mateenali66 August 19, 2026 19:25

owent added a commit to owent/opentelemetry-cpp-contrib that referenced this pull request Aug 20, 2026

Merge concunrrency fixes from <open-telemetry/opentelemetry-cpp#4365>

4156383

owent reviewed Aug 20, 2026

View reviewed changes

mike-nugent-mongodb added 2 commits August 20, 2026 13:02

Break after AfterWait

6b7a1db

Merge remote-tracking branch 'fork/main' into exporter-shutdown-race

18f3bd4

mike-nugent-mongodb commented Aug 20, 2026

View reviewed changes

mike-nugent-mongodb requested a review from owent August 20, 2026 16:51

Uh oh!

Conversation

mike-nugent-mongodb commented Aug 5, 2026

Changes

Uh oh!

linux-foundation-easycla Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mateenali66 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-nugent-mongodb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateenali66 commented Aug 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-nugent-mongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla Bot commented Aug 5, 2026 •

edited

Loading

codecov Bot commented Aug 8, 2026 •

edited

Loading

mike-nugent-mongodb left a comment •

edited

Loading