From 4cafa18fa415f53798440c572243aae8768d3201 Mon Sep 17 00:00:00 2001
From: "J. Nick Koston" <nick@koston.org>
Date: Sun, 6 Jul 2025 19:46:23 -0500
Subject: [PATCH] fix another race

---
 esphome/core/scheduler.cpp | 22 ++++++++++++++++------
 esphome/core/scheduler.h   |  8 +++++++-
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp
index 0c4a4ff230..073eeb4a45 100644
--- a/esphome/core/scheduler.cpp
+++ b/esphome/core/scheduler.cpp
@@ -383,17 +383,27 @@ void HOT Scheduler::process_to_add() {
   this->to_add_.clear();
 }
 void HOT Scheduler::cleanup_() {
+  // Fast path: if nothing to remove, just return
+  // Reading to_remove_ without lock is safe because:
+  // 1. It's volatile, ensuring we read the latest value
+  // 2. If it's 0, there's definitely nothing to cleanup
+  // 3. If it becomes non-zero after we check, cleanup will happen next time
+  if (this->to_remove_ == 0)
+    return;
+
+  // We must hold the lock for the entire cleanup operation because:
+  // 1. We're modifying items_ (via pop_raw_) which other threads may be reading/writing
+  // 2. We're decrementing to_remove_ which must be synchronized with increments
+  // 3. We need a consistent view of items_ throughout the iteration
+  // 4. Other threads might be adding items or modifying the heap structure
+  // Without the lock, we could have race conditions leading to crashes or corruption
+  LockGuard guard{this->lock_};
   while (!this->items_.empty()) {
     auto &item = this->items_[0];
     if (!item->remove)
       return;
-
     this->to_remove_--;
-
-    {
-      LockGuard guard{this->lock_};
-      this->pop_raw_();
-    }
+    this->pop_raw_();
   }
 }
 void HOT Scheduler::pop_raw_() {
diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h
index 79a411db92..3bd4009d27 100644
--- a/esphome/core/scheduler.h
+++ b/esphome/core/scheduler.h
@@ -185,6 +185,12 @@ class Scheduler {
     return item->remove || (item->component != nullptr && item->component->is_failed());
   }
 
+  // Check if the scheduler has no items.
+  // IMPORTANT: This method should only be called from the main thread (loop task).
+  // It performs cleanup of removed items and checks if the queue is empty.
+  // The items_.empty() check at the end is done without a lock for performance,
+  // which is safe because this is only called from the main thread while other
+  // threads only add items (never remove them).
   bool empty_() {
     this->cleanup_();
     return this->items_.empty();
@@ -202,7 +208,7 @@ class Scheduler {
 #endif
   uint32_t last_millis_{0};
   uint16_t millis_major_{0};
-  uint32_t to_remove_{0};
+  volatile uint32_t to_remove_{0};
 };
 
 }  // namespace esphome