reset a child's status when its halt() throws in ControlNode::haltChild (#1273)

* 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.

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Davide Faconti <davide.faconti@gmail.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Author: 3 people

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)

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

tests/gtest_halt_child.cpp

@@ -0,0 +1,122 @@
+#include "behaviortree_cpp/bt_factory.h"
+#include "behaviortree_cpp/control_node.h"
+
+#include <stdexcept>
+
+#include <gtest/gtest.h>
+
+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>("ThrowOnHalt", &halts);
+  factory.registerNodeType<HaltsChild>("HaltsChild");
+  auto tree = factory.createTreeFromText(R"(
+    <root BTCPP_format="4"><BehaviorTree>
+      <HaltsChild><ThrowOnHalt/></HaltsChild>
+    </BehaviorTree></root>)");
+
+  EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING);
+  auto* node = dynamic_cast<HaltsChild*>(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>("ThrowOnHalt", &halts);
+  auto tree =
+      factory.createTreeFromText(std::string(R"(<root BTCPP_format="4"><BehaviorTree>)") +
+                                 body + "</BehaviorTree></root>");
+
+  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("<Sequence><ThrowOnHalt/></Sequence>");
+}
+
+TEST(ControlNodeHaltChild, ThrowingHaltInRetryDecorator)
+{
+  checkBuiltinParentThrowingHalt(
+      R"(<RetryUntilSuccessful num_attempts="3"><ThrowOnHalt/></RetryUntilSuccessful>)");
+}