From e96b66a9d73eb05ea24f90dc68260346bbfa4aeb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 16 Oct 2025 12:15:31 -1000 Subject: [PATCH] [script] BREAKING: Fix unbounded queue growth, optimize queued mode (default max_runs=5) --- esphome/components/script/__init__.py | 7 +++- esphome/components/script/script.h | 59 +++++++++++++++++++-------- esphome/core/helpers.h | 1 + 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/esphome/components/script/__init__.py b/esphome/components/script/__init__.py index e8a8aa5671..58f901b46d 100644 --- a/esphome/components/script/__init__.py +++ b/esphome/components/script/__init__.py @@ -45,8 +45,13 @@ def get_script(script_id): def check_max_runs(value): + # Set default for queued mode to prevent unbounded queue growth + if CONF_MAX_RUNS not in value and value[CONF_MODE] == CONF_QUEUED: + value[CONF_MAX_RUNS] = 5 + if CONF_MAX_RUNS not in value: return value + if value[CONF_MODE] not in [CONF_QUEUED, CONF_PARALLEL]: raise cv.Invalid( "The option 'max_runs' is only valid in 'queue' and 'parallel' mode.", @@ -106,7 +111,7 @@ CONFIG_SCHEMA = automation.validate_automation( cv.Optional(CONF_MODE, default=CONF_SINGLE): cv.one_of( *SCRIPT_MODES, lower=True ), - cv.Optional(CONF_MAX_RUNS): cv.positive_int, + cv.Optional(CONF_MAX_RUNS): cv.int_range(min=1, max=100), cv.Optional(CONF_PARAMETERS, default={}): cv.Schema( { validate_parameter_name: validate_parameter_type, diff --git a/esphome/components/script/script.h b/esphome/components/script/script.h index b87402f52e..4196018dac 100644 --- a/esphome/components/script/script.h +++ b/esphome/components/script/script.h @@ -2,9 +2,8 @@ #include "esphome/core/automation.h" #include "esphome/core/component.h" +#include "esphome/core/helpers.h" #include "esphome/core/log.h" - -#include namespace esphome { namespace script { @@ -96,14 +95,36 @@ template class RestartScript : public Script { /** A script type that queues new instances that are created. * * Only one instance of the script can be active at a time. + * + * Ring buffer implementation: + * - num_queued_ tracks the number of queued (waiting) instances, NOT including the currently running one + * - queue_front_ points to the next item to execute (read position) + * - Buffer size is (max_runs_ - 1) since one instance is always running (not queued) + * - Write position is calculated as: (queue_front_ + num_queued_) % (max_runs_ - 1) + * - When an item finishes, queue_front_ advances: (queue_front_ + 1) % (max_runs_ - 1) + * - First execute() runs immediately without queuing (num_queued_ stays 0) + * - Subsequent executes while running are queued starting at position 0 + * - Maximum total instances = max_runs_ (one running + max_runs_-1 queued) */ template class QueueingScript : public Script, public Component { public: void execute(Ts... x) override { - if (this->is_action_running() || this->num_runs_ > 0) { - // num_runs_ is the number of *queued* instances, so total number of instances is - // num_runs_ + 1 - if (this->max_runs_ != 0 && this->num_runs_ + 1 >= this->max_runs_) { + // Lazy init on first use - avoids setup() ordering issues and saves memory + // if script is never executed during this boot cycle + if (this->var_queue_.capacity() == 0) { + // Allocate max_runs_ - 1 slots since one instance is always running (not queued) + this->var_queue_.init(this->max_runs_ - 1); + // Initialize all unique_ptr slots to nullptr + for (int i = 0; i < this->max_runs_ - 1; i++) { + this->var_queue_.push_back(nullptr); + } + } + + if (this->is_action_running() || this->num_queued_ > 0) { + // num_queued_ is the number of *queued* instances (waiting, not including currently running) + // Total active instances = 1 (running) + num_queued_ (queued) + // So we reject when num_queued_ + 1 >= max_runs_ + if (this->num_queued_ + 1 >= this->max_runs_) { this->esp_logw_(__LINE__, ESPHOME_LOG_FORMAT("Script '%s' maximum number of queued runs exceeded!"), LOG_STR_ARG(this->name_)); return; @@ -111,8 +132,11 @@ template class QueueingScript : public Script, public Com this->esp_logd_(__LINE__, ESPHOME_LOG_FORMAT("Script '%s' queueing new instance (mode: queued)"), LOG_STR_ARG(this->name_)); - this->num_runs_++; - this->var_queue_.push(std::make_tuple(x...)); + // Ring buffer: write to (queue_front_ + num_queued_) % (max_runs_ - 1) + size_t write_pos = (this->queue_front_ + this->num_queued_) % (this->max_runs_ - 1); + // Use reset() to replace the unique_ptr + this->var_queue_[write_pos].reset(new std::tuple(std::make_tuple(x...))); + this->num_queued_++; return; } @@ -122,15 +146,17 @@ template class QueueingScript : public Script, public Com } void stop() override { - this->num_runs_ = 0; + this->num_queued_ = 0; + this->queue_front_ = 0; Script::stop(); } void loop() override { - if (this->num_runs_ != 0 && !this->is_action_running()) { - this->num_runs_--; - auto &vars = this->var_queue_.front(); - this->var_queue_.pop(); + if (this->num_queued_ != 0 && !this->is_action_running()) { + // Dequeue: decrement count, read from front, advance read position + this->num_queued_--; + auto &vars = *this->var_queue_[this->queue_front_]; + this->queue_front_ = (this->queue_front_ + 1) % (this->max_runs_ - 1); this->trigger_tuple_(vars, typename gens::type()); } } @@ -142,9 +168,10 @@ template class QueueingScript : public Script, public Com this->trigger(std::get(tuple)...); } - int num_runs_ = 0; - int max_runs_ = 0; - std::queue> var_queue_; + int num_queued_ = 0; // Number of queued instances (not including currently running) + int max_runs_ = 0; // Maximum total instances (running + queued) + size_t queue_front_ = 0; // Ring buffer read position (next item to execute) + FixedVector>> var_queue_; // Ring buffer of queued parameters }; /** A script type that executes new instances in parallel. diff --git a/esphome/core/helpers.h b/esphome/core/helpers.h index 326718e974..dd67836653 100644 --- a/esphome/core/helpers.h +++ b/esphome/core/helpers.h @@ -298,6 +298,7 @@ template class FixedVector { const T &back() const { return data_[size_ - 1]; } size_t size() const { return size_; } + size_t capacity() const { return capacity_; } bool empty() const { return size_ == 0; } /// Access element without bounds checking (matches std::vector behavior)