From 95ff70eb4896919bae7d36a4c358abae6e68350a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 11:25:14 -1000 Subject: [PATCH 1/5] [mqtt] Fix data race on inbound event queue The ESP-IDF MQTT client dispatches events from its own task, which pushed to a std::queue while the main loop popped from it. std::queue is not thread-safe; concurrent access can corrupt its internal state. Replace with EventPool + LockFreeQueue (SPSC ring buffer) already used elsewhere in the codebase. The pool is sized to queue capacity (SIZE-1) so allocate() fails before push() can, which prevents both a slot leak and an SPSC violation on the pool's free list. Also rename the outbound pool from mqtt_event_pool_ to mqtt_outbound_pool_ to avoid confusion with the new inbound pool. --- .../components/mqtt/mqtt_backend_esp32.cpp | 34 +++++++--- esphome/components/mqtt/mqtt_backend_esp32.h | 68 +++++++++++-------- 2 files changed, 65 insertions(+), 37 deletions(-) diff --git a/esphome/components/mqtt/mqtt_backend_esp32.cpp b/esphome/components/mqtt/mqtt_backend_esp32.cpp index 5642fd5f7b..ae3d06a28f 100644 --- a/esphome/components/mqtt/mqtt_backend_esp32.cpp +++ b/esphome/components/mqtt/mqtt_backend_esp32.cpp @@ -82,10 +82,16 @@ bool MQTTBackendESP32::initialize_() { void MQTTBackendESP32::loop() { // process new events // handle only 1 message per loop iteration - if (!mqtt_events_.empty()) { - auto &event = mqtt_events_.front(); - mqtt_event_handler_(event); - mqtt_events_.pop(); + Event *event = this->mqtt_event_queue_.pop(); + if (event != nullptr) { + mqtt_event_handler_(*event); + this->mqtt_event_pool_.release(event); + } + + // Log dropped inbound events (check is cheap - single atomic load in common case) + uint16_t inbound_dropped = this->mqtt_event_queue_.get_and_reset_dropped_count(); + if (inbound_dropped > 0) { + ESP_LOGW(TAG, "Dropped %u inbound MQTT events", inbound_dropped); } #if defined(USE_MQTT_IDF_ENQUEUE) @@ -183,10 +189,18 @@ void MQTTBackendESP32::mqtt_event_handler_(const Event &event) { void MQTTBackendESP32::mqtt_event_handler(void *handler_args, esp_event_base_t base, int32_t event_id, void *event_data) { MQTTBackendESP32 *instance = static_cast(handler_args); - // queue event to decouple processing + // queue event to decouple processing from ESP-IDF MQTT task to main loop if (instance) { - auto event = *static_cast(event_data); - instance->mqtt_events_.emplace(event); + auto *event = instance->mqtt_event_pool_.allocate(); + if (event == nullptr) { + // Pool exhausted, drop event (counted via queue's dropped counter) + instance->mqtt_event_queue_.increment_dropped_count(); + return; + } + event->populate(*static_cast(event_data)); + // Push always succeeds: pool is sized to queue capacity (N-1), so if + // allocate() returned non-null, the queue is guaranteed to have room. + instance->mqtt_event_queue_.push(event); // Wake main loop immediately to process MQTT event instead of waiting for select() timeout #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) @@ -226,14 +240,14 @@ void MQTTBackendESP32::esphome_mqtt_task(void *params) { break; } } - this_mqtt->mqtt_event_pool_.release(elem); + this_mqtt->mqtt_outbound_pool_.release(elem); } } } bool MQTTBackendESP32::enqueue_(MqttQueueTypeT type, const char *topic, int qos, bool retain, const char *payload, size_t len) { - auto *elem = this->mqtt_event_pool_.allocate(); + auto *elem = this->mqtt_outbound_pool_.allocate(); if (!elem) { // Queue is full - increment counter but don't log immediately. @@ -253,7 +267,7 @@ bool MQTTBackendESP32::enqueue_(MqttQueueTypeT type, const char *topic, int qos, // Use the helper to allocate and copy data if (!elem->set_data(topic, payload, len)) { // Allocation failed, return elem to pool - this->mqtt_event_pool_.release(elem); + this->mqtt_outbound_pool_.release(elem); // Increment counter without logging to avoid cascade effect during memory pressure this->mqtt_queue_.increment_dropped_count(); return false; diff --git a/esphome/components/mqtt/mqtt_backend_esp32.h b/esphome/components/mqtt/mqtt_backend_esp32.h index 5c4dc413bd..fb6d380fbd 100644 --- a/esphome/components/mqtt/mqtt_backend_esp32.h +++ b/esphome/components/mqtt/mqtt_backend_esp32.h @@ -5,7 +5,6 @@ #ifdef USE_ESP32 #include -#include #include #include #include @@ -18,32 +17,39 @@ namespace esphome::mqtt { struct Event { - esp_mqtt_event_id_t event_id; + esp_mqtt_event_id_t event_id{}; std::vector data; - int total_data_len; - int current_data_offset; + int total_data_len{0}; + int current_data_offset{0}; std::string topic; - int msg_id; - bool retain; - int qos; - bool dup; - bool session_present; - esp_mqtt_error_codes_t error_handle; + int msg_id{0}; + bool retain{false}; + int qos{0}; + bool dup{false}; + bool session_present{false}; + esp_mqtt_error_codes_t error_handle{}; - // Construct from esp_mqtt_event_t - // Any pointer values that are unsafe to keep are converted to safe copies - Event(const esp_mqtt_event_t &event) - : event_id(event.event_id), - data(event.data, event.data + event.data_len), - total_data_len(event.total_data_len), - current_data_offset(event.current_data_offset), - topic(event.topic, event.topic_len), - msg_id(event.msg_id), - retain(event.retain), - qos(event.qos), - dup(event.dup), - session_present(event.session_present), - error_handle(*event.error_handle) {} + // Populate from esp_mqtt_event_t + // Copies pointer-based data to owned storage for safe cross-thread transfer + void populate(const esp_mqtt_event_t &event) { + this->event_id = event.event_id; + this->data.assign(event.data, event.data + event.data_len); + this->total_data_len = event.total_data_len; + this->current_data_offset = event.current_data_offset; + this->topic.assign(event.topic, event.topic_len); + this->msg_id = event.msg_id; + this->retain = event.retain; + this->qos = event.qos; + this->dup = event.dup; + this->session_present = event.session_present; + this->error_handle = *event.error_handle; + } + + // Release owned resources for pool reuse (keeps allocated capacity for efficiency) + void release() { + this->data.clear(); + this->topic.clear(); + } }; enum MqttQueueTypeT : uint8_t { @@ -118,7 +124,8 @@ class MQTTBackendESP32 final : public MQTTBackend { static constexpr size_t TASK_STACK_SIZE = 3072; static constexpr size_t TASK_STACK_SIZE_TLS = 4096; // Larger stack for TLS operations static constexpr ssize_t TASK_PRIORITY = 5; - static constexpr uint8_t MQTT_QUEUE_LENGTH = 30; // 30*12 bytes = 360 + static constexpr uint8_t MQTT_QUEUE_LENGTH = 30; // 30*12 bytes = 360 + static constexpr uint8_t MQTT_EVENT_QUEUE_LENGTH = 32; // Inbound events from broker void set_keep_alive(uint16_t keep_alive) final { this->keep_alive_ = keep_alive; } void set_client_id(const char *client_id) final { this->client_id_ = client_id; } @@ -251,7 +258,7 @@ class MQTTBackendESP32 final : public MQTTBackend { bool skip_cert_cn_check_{false}; #if defined(USE_MQTT_IDF_ENQUEUE) static void esphome_mqtt_task(void *params); - EventPool mqtt_event_pool_; + EventPool mqtt_outbound_pool_; NotifyingLockFreeQueue mqtt_queue_; TaskHandle_t task_handle_{nullptr}; bool enqueue_(MqttQueueTypeT type, const char *topic, int qos = 0, bool retain = false, const char *payload = NULL, @@ -266,7 +273,14 @@ class MQTTBackendESP32 final : public MQTTBackend { CallbackManager on_message_; CallbackManager on_publish_; std::string cached_topic_; - std::queue mqtt_events_; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements (one slot distinguishes full from empty). + // This guarantees allocate() returns nullptr before push() can fail, which: + // 1. Prevents leaking a pool slot (the Nth allocate succeeds but push fails) + // 2. Ensures only the main loop ever calls release(), preserving the SPSC + // contract on the pool's internal free list + EventPool mqtt_event_pool_; + LockFreeQueue mqtt_event_queue_; #if defined(USE_MQTT_IDF_ENQUEUE) uint32_t last_dropped_log_time_{0}; From a078d1b14d7d06e3af8a9ae6f5a671cdaa8ce44d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 11:28:58 -1000 Subject: [PATCH 2/5] [esp32_ble] Fix EventPool/LockFreeQueue sizing off-by-one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LockFreeQueue is a ring buffer that holds N-1 elements (one slot is reserved to distinguish full from empty). With the pool also sized to N, the Nth allocate() succeeds but push() fails — permanently leaking one pool slot since the element is never returned. Size the pool to N-1 to match queue capacity. This guarantees allocate() returns nullptr before push() can fail. --- esphome/components/esp32_ble/ble.cpp | 3 ++- esphome/components/esp32_ble/ble.h | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/esphome/components/esp32_ble/ble.cpp b/esphome/components/esp32_ble/ble.cpp index ff9d9bb15a..fee1c546be 100644 --- a/esphome/components/esp32_ble/ble.cpp +++ b/esphome/components/esp32_ble/ble.cpp @@ -575,8 +575,9 @@ template void enqueue_ble_event(Args... args) { load_ble_event(event, args...); // Push the event to the queue + // Push always succeeds: pool is sized to queue capacity (N-1), so if + // allocate() returned non-null, the queue is guaranteed to have room. global_ble->ble_events_.push(event); - // Push always succeeds because we're the only producer and the pool ensures we never exceed queue size } // Explicit template instantiations for the friend function diff --git a/esphome/components/esp32_ble/ble.h b/esphome/components/esp32_ble/ble.h index 04bec3f785..752ddc9d1f 100644 --- a/esphome/components/esp32_ble/ble.h +++ b/esphome/components/esp32_ble/ble.h @@ -221,7 +221,13 @@ class ESP32BLE : public Component { // Large objects (size depends on template parameters, but typically aligned to 4 bytes) esphome::LockFreeQueue ble_events_; - esphome::EventPool ble_event_pool_; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements (one slot distinguishes full from empty). + // This guarantees allocate() returns nullptr before push() can fail, which: + // 1. Prevents leaking a pool slot (the Nth allocate succeeds but push fails) + // 2. Avoids needing release() on the producer path after a failed push(), + // preserving the SPSC contract on the pool's internal free list + esphome::EventPool ble_event_pool_; // 4-byte aligned members #ifdef USE_ESP32_BLE_ADVERTISING From 61513d3debb720cdb73d28d6c0af132f8fe95b1c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 11:30:59 -1000 Subject: [PATCH 3/5] [espnow] Fix EventPool/LockFreeQueue sizing off-by-one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LockFreeQueue is a ring buffer that holds N-1 elements (one slot is reserved to distinguish full from empty). With the pool also sized to N, the Nth allocate() succeeds but push() fails — permanently leaking one pool slot since the element is never returned. Size both receive and send pools to N-1 to match queue capacity. --- esphome/components/espnow/espnow_component.cpp | 6 ++++-- esphome/components/espnow/espnow_component.h | 8 ++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/esphome/components/espnow/espnow_component.cpp b/esphome/components/espnow/espnow_component.cpp index 991803d870..78916891f4 100644 --- a/esphome/components/espnow/espnow_component.cpp +++ b/esphome/components/espnow/espnow_component.cpp @@ -87,7 +87,8 @@ void on_send_report(const uint8_t *mac_addr, esp_now_send_status_t status) // Push the packet to the queue global_esp_now->receive_packet_queue_.push(packet); - // Push always because we're the only producer and the pool ensures we never exceed queue size + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. // Wake main loop immediately to process ESP-NOW send event instead of waiting for select() timeout #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) @@ -109,7 +110,8 @@ void on_data_received(const esp_now_recv_info_t *info, const uint8_t *data, int // Push the packet to the queue global_esp_now->receive_packet_queue_.push(packet); - // Push always because we're the only producer and the pool ensures we never exceed queue size + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. // Wake main loop immediately to process ESP-NOW receive event instead of waiting for select() timeout #if defined(USE_SOCKET_SELECT_SUPPORT) && defined(USE_WAKE_LOOP_THREADSAFE) diff --git a/esphome/components/espnow/espnow_component.h b/esphome/components/espnow/espnow_component.h index 9941e97227..ee4adc1b4d 100644 --- a/esphome/components/espnow/espnow_component.h +++ b/esphome/components/espnow/espnow_component.h @@ -163,10 +163,14 @@ class ESPNowComponent : public Component { uint8_t own_address_[ESP_NOW_ETH_ALEN]{0}; LockFreeQueue receive_packet_queue_{}; - EventPool receive_packet_pool_{}; + // Pool sized to queue capacity (SIZE-1) because LockFreeQueue is a ring + // buffer that holds N-1 elements. This guarantees allocate() returns nullptr + // before push() can fail, preventing a pool slot leak. + EventPool receive_packet_pool_{}; LockFreeQueue send_packet_queue_{}; - EventPool send_packet_pool_{}; + // Pool sized to queue capacity (SIZE-1) — see receive_packet_pool_ comment. + EventPool send_packet_pool_{}; ESPNowSendPacket *current_send_packet_{nullptr}; // Currently sending packet, nullptr if none uint8_t wifi_channel_{0}; From da1ee1bf6635765a6b9de87b97e449e6881f3e65 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 11:39:47 -1000 Subject: [PATCH 4/5] [mqtt] Fix outbound EventPool/LockFreeQueue sizing off-by-one Size the outbound pool to N-1 to match queue capacity, same as the inbound pool fix in the previous commit. --- esphome/components/mqtt/mqtt_backend_esp32.cpp | 4 ++-- esphome/components/mqtt/mqtt_backend_esp32.h | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/esphome/components/mqtt/mqtt_backend_esp32.cpp b/esphome/components/mqtt/mqtt_backend_esp32.cpp index ae3d06a28f..fbfaeb4c5b 100644 --- a/esphome/components/mqtt/mqtt_backend_esp32.cpp +++ b/esphome/components/mqtt/mqtt_backend_esp32.cpp @@ -198,8 +198,8 @@ void MQTTBackendESP32::mqtt_event_handler(void *handler_args, esp_event_base_t b return; } event->populate(*static_cast(event_data)); - // Push always succeeds: pool is sized to queue capacity (N-1), so if - // allocate() returned non-null, the queue is guaranteed to have room. + // Push always succeeds: pool is sized to queue capacity (SIZE-1), so if + // allocate() returned non-null, the queue cannot be full. instance->mqtt_event_queue_.push(event); // Wake main loop immediately to process MQTT event instead of waiting for select() timeout diff --git a/esphome/components/mqtt/mqtt_backend_esp32.h b/esphome/components/mqtt/mqtt_backend_esp32.h index fb6d380fbd..58d1b29b32 100644 --- a/esphome/components/mqtt/mqtt_backend_esp32.h +++ b/esphome/components/mqtt/mqtt_backend_esp32.h @@ -258,7 +258,8 @@ class MQTTBackendESP32 final : public MQTTBackend { bool skip_cert_cn_check_{false}; #if defined(USE_MQTT_IDF_ENQUEUE) static void esphome_mqtt_task(void *params); - EventPool mqtt_outbound_pool_; + // Pool sized to queue capacity (SIZE-1) — see mqtt_event_pool_ comment. + EventPool mqtt_outbound_pool_; NotifyingLockFreeQueue mqtt_queue_; TaskHandle_t task_handle_{nullptr}; bool enqueue_(MqttQueueTypeT type, const char *topic, int qos = 0, bool retain = false, const char *payload = NULL, @@ -277,8 +278,8 @@ class MQTTBackendESP32 final : public MQTTBackend { // buffer that holds N-1 elements (one slot distinguishes full from empty). // This guarantees allocate() returns nullptr before push() can fail, which: // 1. Prevents leaking a pool slot (the Nth allocate succeeds but push fails) - // 2. Ensures only the main loop ever calls release(), preserving the SPSC - // contract on the pool's internal free list + // 2. Avoids needing release() on the producer path after a failed push(), + // preserving the SPSC contract on the pool's internal free list EventPool mqtt_event_pool_; LockFreeQueue mqtt_event_queue_; From 8acc033873a93127a96bf2e0628f74cae2389405 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 17 Mar 2026 11:58:47 -1000 Subject: [PATCH 5/5] [core] Document EventPool sizing requirement with LockFreeQueue Add a comment to EventPool documenting that when paired with a LockFreeQueue, the pool should be sized to N-1 (the queue's actual capacity) to prevent slot leaks and SPSC violations. --- esphome/core/event_pool.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/esphome/core/event_pool.h b/esphome/core/event_pool.h index 99541d4a17..9d3d3d7d0e 100644 --- a/esphome/core/event_pool.h +++ b/esphome/core/event_pool.h @@ -13,6 +13,13 @@ namespace esphome { // Events are allocated on first use and reused thereafter, growing to peak usage // @tparam T The type of objects managed by the pool (must have a release() method) // @tparam SIZE The maximum number of objects in the pool (1-255, limited by uint8_t) +// +// SIZING: When paired with a LockFreeQueue, the pool SIZE should be +// Q_SIZE - 1 (the queue's actual capacity, since the ring buffer reserves one slot). +// This ensures allocate() returns nullptr before push() can fail, which: +// - Prevents permanently leaking a pool slot +// - Avoids needing release() on the producer path after a failed push(), +// preserving the SPSC contract on the internal free list template class EventPool { public: EventPool() : total_created_(0) {}