[core] Refactor scheduler to eliminate hidden side effects in empty_ (#9743)

esphome/core/scheduler.cpp

@@ -219,10 +219,13 @@ bool HOT Scheduler::cancel_retry(Component *component, const std::string &name)
 
 optional<uint32_t> HOT Scheduler::next_schedule_in(uint32_t now) {
   // IMPORTANT: This method should only be called from the main thread (loop task).
-  // It calls empty_() and accesses items_[0] without holding a lock, which is only
+  // It performs cleanup and accesses items_[0] without holding a lock, which is only
   // safe when called from the main thread. Other threads must not call this method.
-  if (this->empty_())
+
+  // If no items, return empty optional
+  if (this->cleanup_() == 0)
     return {};
+
   auto &item = this->items_[0];
   // Convert the fresh timestamp from caller (usually Application::loop()) to 64-bit
   const auto now_64 = this->millis_64_(now);  // 'now' from parameter - fresh from caller
@@ -282,7 +285,9 @@ void HOT Scheduler::call(uint32_t now) {
     ESP_LOGD(TAG, "Items: count=%zu, now=%" PRIu64 " (%" PRIu16 ", %" PRIu32 ")", this->items_.size(), now_64,
              this->millis_major_, this->last_millis_);
 #endif /* else ESPHOME_CORES_MULTI_ATOMICS */
-    while (!this->empty_()) {
+    // Cleanup before debug output
+    this->cleanup_();
+    while (!this->items_.empty()) {
       std::unique_ptr<SchedulerItem> item;
       {
         LockGuard guard{this->lock_};
@@ -333,7 +338,9 @@ void HOT Scheduler::call(uint32_t now) {
     this->to_remove_ = 0;
   }
 
-  while (!this->empty_()) {
+  // Cleanup removed items before processing
+  this->cleanup_();
+  while (!this->items_.empty()) {
     // use scoping to indicate visibility of `item` variable
     {
       // Don't copy-by value yet
@@ -399,8 +406,8 @@ void HOT Scheduler::process_to_add() {
   }
   this->to_add_.clear();
 }
-void HOT Scheduler::cleanup_() {
+size_t HOT Scheduler::cleanup_() {
-  // Fast path: if nothing to remove, just return
+  // Fast path: if nothing to remove, just return the current size
   // Reading to_remove_ without lock is safe because:
   // 1. We only call this from the main thread during call()
   // 2. If it's 0, there's definitely nothing to cleanup
@@ -408,7 +415,7 @@ void HOT Scheduler::cleanup_() {
   // 4. Not all platforms support atomics, so we accept this race in favor of performance
   // 5. The worst case is a one-loop-iteration delay in cleanup, which is harmless
   if (this->to_remove_ == 0)
-    return;
+    return this->items_.size();
 
   // We must hold the lock for the entire cleanup operation because:
   // 1. We're modifying items_ (via pop_raw_) which requires exclusive access
@@ -422,10 +429,11 @@ void HOT Scheduler::cleanup_() {
   while (!this->items_.empty()) {
     auto &item = this->items_[0];
     if (!item->remove)
-      return;
+      break;
     this->to_remove_--;
     this->pop_raw_();
   }
+  return this->items_.size();
 }
 void HOT Scheduler::pop_raw_() {
   std::pop_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp);

esphome/core/scheduler.h

@@ -58,6 +58,9 @@ class Scheduler {
 
   // Calculate when the next scheduled item should run
   // @param now Fresh timestamp from millis() - must not be stale/cached
+  // Returns the time in milliseconds until the next scheduled item, or nullopt if no items
+  // This method performs cleanup of removed items before checking the schedule
+  // IMPORTANT: This method should only be called from the main thread (loop task).
   optional<uint32_t> next_schedule_in(uint32_t now);
 
   // Execute all scheduled items that are ready
@@ -147,7 +150,10 @@ class Scheduler {
                          uint32_t delay, std::function<void()> func);
 
   uint64_t millis_64_(uint32_t now);
-  void cleanup_();
+  // Cleanup logically deleted items from the scheduler
+  // Returns the number of items remaining after cleanup
+  // IMPORTANT: This method should only be called from the main thread (loop task).
+  size_t cleanup_();
   void pop_raw_();
 
  private:
@@ -191,17 +197,6 @@ 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();
-  }
-
   Mutex lock_;
   std::vector<std::unique_ptr<SchedulerItem>> items_;
   std::vector<std::unique_ptr<SchedulerItem>> to_add_;