Skip to content

[core] Free the action/condition tree when an Automation is destroyed - #71

Merged
zkoalexey merged 1 commit into
dev-jethubfrom
fix/automation
Aug 28, 2026
Merged

[core] Free the action/condition tree when an Automation is destroyed#71
zkoalexey merged 1 commit into
dev-jethubfrom
fix/automation

Conversation

@zkoalexey

@zkoalexey zkoalexey commented Aug 17, 2026

Copy link
Copy Markdown

What

~Automation freed its triggers and nothing else. The action list, every action
in it, and every condition hanging off those actions were leaked.

Upstream that is harmless — code-generated automations are newed once and never
destroyed. It is not harmless here: the automations component rebuilds its
runtime object graph on every save from the web editor, and on every
remove_automation() / reset_all().

The change

core/automation.h

  • virtual ~Action(), virtual ~Condition() — deleting through those bases was
    UB before.
  • ~ActionList() walks actions_begin_ / next_ and deletes the chain. The list
    owns what it is given; copy construction/assignment are deleted, since a copy
    would free the same chain twice. Automation::actions_ is a by-value member, so
    ~Automation now frees the whole tree without any change of its own.

core/base_automation.h — the owners free what they hold: IfAction and
WhileAction their condition, ForCondition and WaitUntilAction theirs, and
AndCondition / OrCondition / XorCondition / NotCondition their children.
then_ / else_ are by-value ActionLists and free themselves.

Three hazards the destructors expose, handled here

Scheduler. DelayAction and WaitUntilAction are Components that arm a
timer against this, and Scheduler::SchedulerItem keeps a raw Component * it
dereferences (is_failed()) while walking its heap. Freeing one with a timer
pending is a use-after-free, not merely a stale callback. Both destructors cancel
first. stop() is not enough on its own: stop_complex() only calls it while
num_running_ != 0, and a runtime build can be torn down on a path that never
calls stop() at all. Both timer names are static strings, so both are
cancellable.

The scheduler dereferenced cancelled items. Cancelling is only half of it.
cancel_item_locked_() unlinks an item outright only when it is items_.back();
everywhere else it just marks it, leaving it in the heap still holding that raw
Component *. The dispatch loop in call() then read
item->component->is_failed() before testing the remove flag, so a cancelled
item belonging to a freed component was dereferenced anyway — and cleanup_()
does not reliably get there first, since it pops only leading removed items and
full compaction needs MAX_LOGICALLY_DELETED_ITEMS. The two checks are now in
the order Scheduler::should_skip_item_() has always used for the defer queue. A
side effect: to_remove_ no longer drifts when an item is both removed and owned
by a failed component.

sprinkler double ownership. SprinklerValve and Sprinkler held their
shutdown / resume-or-start actions in unique_ptrs and handed the same raw
pointers to an Automation's ActionList. With an owning ActionList that is a
double free. The ActionList is now the sole owner and the redundant members are
gone; nothing else read them. Dormant before this change — a Sprinkler is a
codegen global whose Automations outlive the process — so this is a latent bug
being closed, not one that was firing.

Blast radius

A behavioural no-op for statically generated automations: they are never
destroyed, so the new destructors never run for them. Action and Condition
already had virtual functions, so the added virtual destructor costs vtable slots
per class, nothing per object.

Every add_action / add_actions / add_then / add_else call site in the tree
was audited for a second owner or a non-heap pointer; sprinkler was the only
one. Codegen (automation.py build_action_list / build_automation) only ever
passes new_Pvariable objects, each into exactly one list.

Verification

  • sprinkler compiled and linked on the host platform from
    tests/components/sprinkler/common.yaml (plus a standby_switch, to reach the
    third edited call site).
  • Full ESP32 device firmware built and booted in QEMU (JetHome workspace,
    jxd-r6-e1eth-lcd); automations created, updated 240×, and deleted over REST
    with no crash and a flat heap.
  • Host GoogleTest coverage lives in the workspace repo that consumes this fork:
    esphome-components/tests/cpp/automations/automation_lifetime_test.cpp — probes
    that count live instances, a scheduler-item check for ~DelayAction, and an
    armed operator new/delete counter over a build+destroy cycle. All seven
    cases fail without this change.

Known limit, documented rather than fixed

Trigger can decline deletion through prepare_for_deletion(); Action and
Condition have no such hook. So "an Action/Condition owned by an ActionList
must never be registered with App" is a call-site rule, spelled out at
~ActionList. Codegen does register DelayAction, WaitUntilAction and
ForCondition — safe only because codegen automations are never destroyed, and a
trap for whoever first builds one of those at run time.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes ESPHome’s C++ automation graph safely destructible by fixing longstanding ownership gaps: actions, action lists, and nested conditions are now properly freed when an Automation is destroyed. It also hardens Scheduler::call() against a use-after-free exposed by these new destruction paths, and removes a latent double-ownership pattern in sprinkler that would become a double-free once action lists own their actions.

Changes:

  • Add virtual destructors to Action and Condition, and make ActionList an owning container that deletes its chained actions on destruction (copy disabled to prevent double-free).
  • Reorder scheduler dispatch logic to skip logically-removed items before dereferencing Component* for is_failed() checks.
  • Remove redundant unique_ptr ownership of sprinkler actions so the Automation action lists are the sole owners.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
esphome/core/scheduler.cpp Avoid dereferencing cancelled scheduler items (raw Component*) before checking the removal flag.
esphome/core/component.cpp Update ISR-safety comment to clarify runtime-built automation edge cases.
esphome/core/base_automation.h Add destructors to free owned conditions/actions and cancel pending timers in DelayAction/WaitUntilAction.
esphome/core/automation.h Add virtual destructors for base types and implement owning ActionList destructor + disable copying.
esphome/components/sprinkler/sprinkler.h Remove duplicate owning members for actions now owned by Automation action lists.
esphome/components/sprinkler/sprinkler.cpp Allocate sprinkler actions directly into Automation action lists (single owner).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

~Automation freed its triggers and nothing else. The action list, every action in
it, and every condition hanging off those actions were leaked. Upstream that is
harmless -- code-generated automations are new'ed once and never destroyed -- but
the JetHome `automations` component rebuilds its runtime object graph on every
save from the web editor, and on every remove/reset.

core/automation.h gets virtual ~Action() and ~Condition() (deleting through those
bases was UB), and an owning ~ActionList() that walks the actions_begin_/next_
chain. Automation::actions_ is a by-value member, so ~Automation now frees the
whole tree without a change of its own. ActionList copy/assign are deleted: a
copy would free the same chain twice.

core/base_automation.h: the owners free what they hold -- IfAction and
WhileAction their condition, ForCondition and WaitUntilAction theirs, and the
And/Or/Xor/Not group conditions their children.

Three hazards the destructors expose, handled here:

- DelayAction and WaitUntilAction are Components that arm a scheduler timer
  against themselves, and Scheduler::SchedulerItem keeps a raw Component* it
  dereferences (is_failed()) while walking its heap. Freeing one with a timer
  pending is a use-after-free, not a stale callback. Both destructors cancel
  first; stop() alone is not enough, since stop_complex() only calls it while
  num_running_ != 0 and a runtime build can be torn down without any stop() at
  all. Both timer names are static strings, so both are cancellable.

- Cancelling is only half of it. cancel_item_locked_() unlinks an item outright
  only when it is items_.back(); everywhere else it just marks it, leaving it in
  the heap still holding that raw Component*. The dispatch loop in call() then
  read item->component->is_failed() *before* testing the remove flag, so a
  cancelled item belonging to a freed component was still dereferenced -- and
  cleanup_() does not necessarily reach it first, since it pops only leading
  removed items and full compaction needs MAX_LOGICALLY_DELETED_ITEMS. The two
  checks are now in the same order Scheduler::should_skip_item_() has always used
  for the defer queue. As a side effect the to_remove_ counter no longer drifts
  when an item is both removed and owned by a failed component.

- sprinkler held its shutdown / resume-or-start actions in unique_ptrs *and*
  handed the same raw pointers to an Automation's ActionList. With an owning
  ActionList that is a double free. The ActionList is now the sole owner and the
  redundant members are gone; nothing else read them. Dormant before this change
  (a Sprinkler is a codegen global whose Automations outlive the process), so
  this closes a latent bug rather than one that was firing.

Every add_action / add_actions / add_then / add_else call site was audited for a
second owner or a non-heap pointer; sprinkler was the only one. Codegen passes
only new_Pvariable objects, each into exactly one list. A behavioural no-op for
statically generated automations.

Known limit, documented at ~ActionList rather than fixed: Trigger can decline
deletion via prepare_for_deletion(), Action and Condition cannot, so "an
Action/Condition owned by an ActionList must never be registered with App" is a
call-site rule. Codegen registers DelayAction, WaitUntilAction and ForCondition,
which is safe only because codegen automations are never destroyed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zkoalexey zkoalexey closed this Aug 28, 2026
@zkoalexey zkoalexey reopened this Aug 28, 2026
@zkoalexey
zkoalexey merged commit 82a03d3 into dev-jethub Aug 28, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants