From fbbcdd188be508b903041a792f4157bf87128195 Mon Sep 17 00:00:00 2001 From: David Vadovszki Date: Tue, 27 Oct 2026 16:07:54 -0700 Subject: [PATCH 1/2] reset a child's status when its halt() throws in ControlNode::haltChild Previously the child stayed RUNNING, so the next halt of the parent halted it again and threw a second time. When that happened inside ~Tree(), the process terminated. Callers still see the exception. This covers haltChild() and haltChildren(), which calls it. ControlNode::resetChildren() and DecoratorNode::resetChild() have the same pattern and are left for a follow-up. --- src/control_node.cpp | 11 ++++- tests/CMakeLists.txt | 1 + tests/gtest_halt_child.cpp | 85 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 tests/gtest_halt_child.cpp diff --git a/src/control_node.cpp b/src/control_node.cpp index 535165376..b28fec77c 100644 --- a/src/control_node.cpp +++ b/src/control_node.cpp @@ -57,7 +57,16 @@ void ControlNode::haltChild(size_t i) auto* child = children_nodes_[i]; if(child->status() == NodeStatus::RUNNING) { - child->haltNode(); + try + { + child->haltNode(); + } + catch(...) + { + // Don't leave the child RUNNING, or the next reset would halt it again. + child->resetStatus(); + throw; + } } child->resetStatus(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ad39d7879..103715ee6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -49,6 +49,7 @@ set(BT_TESTS gtest_switch.cpp gtest_tree.cpp gtest_try_catch.cpp + gtest_halt_child.cpp gtest_exception_tracking.cpp gtest_updates.cpp gtest_wakeup.cpp diff --git a/tests/gtest_halt_child.cpp b/tests/gtest_halt_child.cpp new file mode 100644 index 000000000..5f4aff13a --- /dev/null +++ b/tests/gtest_halt_child.cpp @@ -0,0 +1,85 @@ +#include "behaviortree_cpp/bt_factory.h" +#include "behaviortree_cpp/control_node.h" + +#include + +#include + +using BT::NodeStatus; + +namespace +{ +// RUNNING until halted, and its onHalted() throws. Counts the halts. +class ThrowOnHalt : public BT::StatefulActionNode +{ +public: + ThrowOnHalt(const std::string& name, const BT::NodeConfig& config, int* halts) + : StatefulActionNode(name, config), halts_(halts) + {} + static BT::PortsList providedPorts() + { + return {}; + } + NodeStatus onStart() override + { + return NodeStatus::RUNNING; + } + NodeStatus onRunning() override + { + return NodeStatus::RUNNING; + } + void onHalted() override + { + (*halts_)++; + throw std::runtime_error("halt failed"); + } + +private: + int* halts_; +}; + +// Ticks its only child and halts it on request through haltChild(). +class HaltsChild : public BT::ControlNode +{ +public: + HaltsChild(const std::string& name, const BT::NodeConfig& config) + : ControlNode(name, config) + {} + static BT::PortsList providedPorts() + { + return {}; + } + NodeStatus tick() override + { + setStatus(NodeStatus::RUNNING); + return children_nodes_[0]->executeTick(); + } + void halt() override + { + haltChild(0); + resetStatus(); + } +}; +} // namespace + +// A child whose halt() throws must not stay RUNNING, or the next haltChild() +// halts it again and throws a second time. +TEST(ControlNodeHaltChild, ThrowingHaltIsNotRepeated) +{ + int halts = 0; + BT::BehaviorTreeFactory factory; + factory.registerNodeType("ThrowOnHalt", &halts); + factory.registerNodeType("HaltsChild"); + auto tree = factory.createTreeFromText(R"( + + + )"); + + EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + auto* node = static_cast(tree.rootNode()); + EXPECT_THROW(node->halt(), std::runtime_error); + EXPECT_EQ(halts, 1); + EXPECT_NO_THROW(node->halt()); + EXPECT_EQ(halts, 1); + EXPECT_EQ(tree.subtrees[0]->nodes[1]->status(), NodeStatus::IDLE); +} From b1a3f86a747822642f934d553b7837cff0198a7e Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 7 Nov 2026 18:07:29 +0100 Subject: [PATCH 2/2] move the throwing-halt status reset into TreeNode::haltNode() The previous commit reset a child's status in ControlNode::haltChild() when its halt() threw, so the next reset does not halt it again. But built-in nodes halt their children through ControlNode::resetChildren() and DecoratorNode::resetChild()/haltChild(), and Tree::haltTree() halts the root directly: all of them go through TreeNode::haltNode() and kept the bug (the node stays RUNNING, is halted a second time, and ~Tree() -> haltTree() throws again from a destructor, i.e. std::terminate). Put the guard in the single choke point, TreeNode::haltNode(): try { halt(); } catch(...) { resetStatus(); throw; }, and drop the now-redundant try/catch in ControlNode::haltChild(). The exception still propagates. The existing _onHalted post-condition code is unchanged. Add tests with built-in parents (Sequence, RetryUntilSuccessful): haltTree() throws once, every node is IDLE afterwards, a second haltTree() does not throw and onHalted() is not called again. No ABI change. Co-Authored-By: Claude Opus 5.5 --- src/control_node.cpp | 11 +---------- src/tree_node.cpp | 12 +++++++++++- tests/gtest_halt_child.cpp | 39 +++++++++++++++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/control_node.cpp b/src/control_node.cpp index b28fec77c..535165376 100644 --- a/src/control_node.cpp +++ b/src/control_node.cpp @@ -57,16 +57,7 @@ void ControlNode::haltChild(size_t i) auto* child = children_nodes_[i]; if(child->status() == NodeStatus::RUNNING) { - try - { - child->haltNode(); - } - catch(...) - { - // Don't leave the child RUNNING, or the next reset would halt it again. - child->resetStatus(); - throw; - } + child->haltNode(); } child->resetStatus(); } diff --git a/src/tree_node.cpp b/src/tree_node.cpp index b0c612f7d..00eb1f595 100644 --- a/src/tree_node.cpp +++ b/src/tree_node.cpp @@ -158,7 +158,17 @@ NodeStatus TreeNode::executeTick() void TreeNode::haltNode() { - halt(); + try + { + halt(); + } + catch(...) + { + // Don't leave the node RUNNING, or the next reset would halt it again + // (and throw again, possibly from a destructor). + resetStatus(); + throw; + } const auto& parse_executor = _p->post_parsed[size_t(PostCond::ON_HALTED)]; if(parse_executor) diff --git a/tests/gtest_halt_child.cpp b/tests/gtest_halt_child.cpp index 5f4aff13a..2e9f4ff1c 100644 --- a/tests/gtest_halt_child.cpp +++ b/tests/gtest_halt_child.cpp @@ -76,10 +76,47 @@ TEST(ControlNodeHaltChild, ThrowingHaltIsNotRepeated) )"); EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING); - auto* node = static_cast(tree.rootNode()); + auto* node = dynamic_cast(tree.rootNode()); EXPECT_THROW(node->halt(), std::runtime_error); EXPECT_EQ(halts, 1); EXPECT_NO_THROW(node->halt()); EXPECT_EQ(halts, 1); EXPECT_EQ(tree.subtrees[0]->nodes[1]->status(), NodeStatus::IDLE); } + +namespace +{ +// Built-in parents halt their children via resetChildren()/resetChild(), not +// haltChild(): the failed child must end up IDLE there too. +void checkBuiltinParentThrowingHalt(const char* body) +{ + int halts = 0; + BT::BehaviorTreeFactory factory; + factory.registerNodeType("ThrowOnHalt", &halts); + auto tree = + factory.createTreeFromText(std::string(R"()") + + body + ""); + + EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING); + EXPECT_ANY_THROW(tree.haltTree()); + EXPECT_EQ(halts, 1); + for(const auto& node : tree.subtrees[0]->nodes) + { + EXPECT_EQ(node->status(), NodeStatus::IDLE) << node->name(); + } + EXPECT_NO_THROW(tree.haltTree()); + EXPECT_EQ(halts, 1); + // ~Tree() calls haltTree() again: it must not throw (-> terminate). +} +} // namespace + +TEST(ControlNodeHaltChild, ThrowingHaltInSequence) +{ + checkBuiltinParentThrowingHalt(""); +} + +TEST(ControlNodeHaltChild, ThrowingHaltInRetryDecorator) +{ + checkBuiltinParentThrowingHalt( + R"()"); +}