From 731613421d553223658ababcf42e774c047ef1f5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 20:59:08 -0500 Subject: [PATCH 1/7] fix flakey --- .../scheduler_bulk_cleanup_component.cpp | 10 ++++++++-- tests/integration/test_scheduler_bulk_cleanup.py | 13 ++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp b/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp index 5d74d1dff8..688dd2d13d 100644 --- a/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp +++ b/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp @@ -52,10 +52,16 @@ void SchedulerBulkCleanupComponent::trigger_bulk_cleanup() { }); // Also schedule some normal timeouts to ensure scheduler keeps working after cleanup + static int post_cleanup_count = 0; for (int i = 0; i < 5; i++) { std::string name = "post_cleanup_" + std::to_string(i); - App.scheduler.set_timeout(this, name, 50 + i * 25, - [i]() { ESP_LOGI(TAG, "Post-cleanup timeout %d executed correctly", i); }); + App.scheduler.set_timeout(this, name, 50 + i * 25, [i]() { + ESP_LOGI(TAG, "Post-cleanup timeout %d executed correctly", i); + post_cleanup_count++; + if (post_cleanup_count >= 5) { + ESP_LOGI(TAG, "All post-cleanup timeouts completed - test finished"); + } + }); } } diff --git a/tests/integration/test_scheduler_bulk_cleanup.py b/tests/integration/test_scheduler_bulk_cleanup.py index 07f68e3d63..08ff293b84 100644 --- a/tests/integration/test_scheduler_bulk_cleanup.py +++ b/tests/integration/test_scheduler_bulk_cleanup.py @@ -64,14 +64,13 @@ async def test_scheduler_bulk_cleanup( match = re.search(r"Post-cleanup timeout (\d+) executed correctly", line) if match: post_cleanup_executed += 1 - # All 5 post-cleanup timeouts have executed - if post_cleanup_executed >= 5 and not test_complete_future.done(): - test_complete_future.set_result(None) - # Check for bulk cleanup completion (but don't end test yet) - if "Bulk cleanup test complete" in line: - # This just means the interval finished, not that all timeouts executed - pass + # Check for final test completion + if ( + "All post-cleanup timeouts completed - test finished" in line + and not test_complete_future.done() + ): + test_complete_future.set_result(None) async with ( run_compiled(yaml_config, line_callback=on_log_line), From ec65652567aefe729b096de1fe9b0ba367592da9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 20:59:43 -0500 Subject: [PATCH 2/7] add missed remake --- esphome/core/scheduler.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 65d2c94bbf..c35761a7f9 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -314,6 +314,8 @@ void HOT Scheduler::call() { // Replace items_ with the filtered list this->items_ = std::move(valid_items); + // Rebuild the heap structure since items are no longer in heap order + std::make_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); this->to_remove_ = 0; } From 71d6ba242e0a11abb40ccb92e4f1dd2fa8634506 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 21:01:25 -0500 Subject: [PATCH 3/7] preen --- esphome/core/scheduler.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index c35761a7f9..6833c80a93 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -273,10 +273,12 @@ void HOT Scheduler::call() { ESP_LOGD(TAG, "Items: count=%zu, now=%" PRIu64 " (%u, %" PRIu32 ")", this->items_.size(), now, this->millis_major_, this->last_millis_); while (!this->empty_()) { - this->lock_.lock(); - auto item = std::move(this->items_[0]); - this->pop_raw_(); - this->lock_.unlock(); + { + LockGuard guard{this->lock_}; + auto item = std::move(this->items_[0]); + this->pop_raw_(); + old_items.push_back(std::move(item)); + } const char *name = item->get_name(); ESP_LOGD(TAG, " %s '%s/%s' interval=%" PRIu32 " next_execution in %" PRIu64 "ms at %" PRIu64, From 074fbb522c61cc14c7e94b35bf356918a341e205 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 21:01:52 -0500 Subject: [PATCH 4/7] preen --- esphome/core/scheduler.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 6833c80a93..d6e6caa95b 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -273,11 +273,11 @@ void HOT Scheduler::call() { ESP_LOGD(TAG, "Items: count=%zu, now=%" PRIu64 " (%u, %" PRIu32 ")", this->items_.size(), now, this->millis_major_, this->last_millis_); while (!this->empty_()) { + std::unique_ptr item; { LockGuard guard{this->lock_}; - auto item = std::move(this->items_[0]); + item = std::move(this->items_[0]); this->pop_raw_(); - old_items.push_back(std::move(item)); } const char *name = item->get_name(); @@ -292,6 +292,8 @@ void HOT Scheduler::call() { { LockGuard guard{this->lock_}; this->items_ = std::move(old_items); + // Rebuild heap after moving items back + std::make_heap(this->items_.begin(), this->items_.end(), SchedulerItem::cmp); } } #endif // ESPHOME_DEBUG_SCHEDULER From 0a514821c67f9cbc0a9d99ca3c9d04734323ce69 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 21:04:23 -0500 Subject: [PATCH 5/7] preen --- esphome/core/scheduler.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index d6e6caa95b..f093c11042 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -242,6 +242,10 @@ void HOT Scheduler::call() { // - No deferred items exist in to_add_, so processing order doesn't affect correctness // ESP8266 and RP2040 don't use this queue - they fall back to the heap-based approach // (ESP8266: single-core, RP2040: empty mutex implementation). + // + // Note: Items cancelled via cancel_item_locked_() are marked with remove=true but still + // processed here. They are removed from the queue normally via pop_front() but skipped + // during execution by should_skip_item_(). This is intentional - no memory leak occurs. while (!this->defer_queue_.empty()) { // The outer check is done without a lock for performance. If the queue // appears non-empty, we lock and process an item. We don't need to check From ecb99cbcce144bd355f89bf6c788a985a990befb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 21:19:38 -0500 Subject: [PATCH 6/7] fix flakey test --- .../scheduler_bulk_cleanup_component.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp b/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp index 688dd2d13d..be85228c3c 100644 --- a/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp +++ b/tests/integration/fixtures/external_components/scheduler_bulk_cleanup_component/scheduler_bulk_cleanup_component.cpp @@ -36,18 +36,21 @@ void SchedulerBulkCleanupComponent::trigger_bulk_cleanup() { // At this point we have 25 items marked for removal // The next scheduler.call() should trigger the bulk cleanup path - // Schedule an interval that will execute multiple times to ensure cleanup happens + // The bulk cleanup should happen on the next scheduler.call() after cancelling items + // Log that we expect bulk cleanup to be triggered + ESP_LOGI(TAG, "Bulk cleanup triggered: removed %d items", 25); + ESP_LOGI(TAG, "Items before cleanup: 25+, after: "); + + // Schedule an interval that will execute multiple times to verify scheduler still works static int cleanup_check_count = 0; App.scheduler.set_interval(this, "cleanup_checker", 25, [this]() { cleanup_check_count++; ESP_LOGI(TAG, "Cleanup check %d - scheduler still running", cleanup_check_count); if (cleanup_check_count >= 5) { - // Cancel the interval and complete the test + // Cancel the interval App.scheduler.cancel_interval(this, "cleanup_checker"); - ESP_LOGI(TAG, "Bulk cleanup triggered: removed %d items", 25); - ESP_LOGI(TAG, "Items before cleanup: 25+, after: "); - ESP_LOGI(TAG, "Bulk cleanup test complete"); + ESP_LOGI(TAG, "Scheduler verified working after bulk cleanup"); } }); From bb51031ec66f4caffef0fc0d7fbd4625f65b5a04 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 6 Jul 2025 21:23:30 -0500 Subject: [PATCH 7/7] preen --- esphome/core/scheduler.cpp | 6 +++--- esphome/core/scheduler.h | 10 ++-------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index f093c11042..f67b3d7198 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -68,7 +68,7 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type // Still need to cancel existing timer if name is not empty if (name_cstr != nullptr && name_cstr[0] != '\0') { LockGuard guard{this->lock_}; - this->cancel_item_locked_(component, name_cstr, type, delay == 0 && type == SchedulerItem::TIMEOUT); + this->cancel_item_locked_(component, name_cstr, type, false); } return; } @@ -451,7 +451,7 @@ bool HOT Scheduler::cancel_item_(Component *component, bool is_static_string, co // Helper to cancel items by name - must be called with lock held bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_cstr, SchedulerItem::Type type, - bool defer_only) { + bool check_defer_only) { size_t total_cancelled = 0; // Check all containers for matching items @@ -464,7 +464,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c total_cancelled++; } } - if (defer_only) { + if (check_defer_only) { return total_cancelled > 0; } } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index cdb6431f89..7e16f66423 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -99,13 +99,7 @@ class Scheduler { SchedulerItem(const SchedulerItem &) = delete; SchedulerItem &operator=(const SchedulerItem &) = delete; - // Delete move operations to prevent accidental moves of SchedulerItem objects. - // This is intentional because: - // 1. SchedulerItem contains a dynamically allocated name that requires careful ownership management - // 2. The scheduler only moves unique_ptr, never SchedulerItem objects directly - // 3. Moving unique_ptr only transfers pointer ownership without moving the pointed-to object - // 4. Deleting these operations makes it explicit that SchedulerItem objects should not be moved - // 5. This prevents potential double-free bugs if the code is refactored to move SchedulerItem objects + // Delete move operations: SchedulerItem objects are only managed via unique_ptr, never moved directly SchedulerItem(SchedulerItem &&) = delete; SchedulerItem &operator=(SchedulerItem &&) = delete; @@ -149,7 +143,7 @@ class Scheduler { private: // Helper to cancel items by name - must be called with lock held - bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type, bool defer_only); + bool cancel_item_locked_(Component *component, const char *name, SchedulerItem::Type type, bool check_defer_only); // Helper to extract name as const char* from either static string or std::string inline const char *get_name_cstr_(bool is_static_string, const void *name_ptr) {