Roll back a rejected XML load as a whole, and keep Tree's destructor from throwing

src/bt_factory.cpp

@@ -720,7 +720,20 @@ std::shared_ptr<WakeUpSignal> Tree::wakeUpSignal() const
 
 Tree::~Tree()
 {
-  haltTree();
+  // Halting may throw (e.g. TryCatch catch_on_halt ticks its catch child), and
+  // an exception escaping a destructor calls std::terminate (issue #1281).
+  try
+  {
+    haltTree();
+  }
+  catch(const std::exception& ex)
+  {
+    std::cerr << "[Tree::~Tree]: haltTree() threw: " << ex.what() << std::endl;
+  }
+  catch(...)
+  {
+    std::cerr << "[Tree::~Tree]: haltTree() threw an unknown exception" << std::endl;
+  }
 }
 
 NodeStatus Tree::tickExactlyOnce()

src/xml_parsing.cpp

@@ -248,21 +248,31 @@ struct XMLParser::PImpl
 
   void loadDocImpl(XMLDocument* doc, bool add_includes);
 
-  // Load the document just appended to opened_documents. A document that is
-  // rejected (XML error, invalid tree) registers no tree, so forget it again:
-  // it would otherwise remain the "first opened document" that
-  // instantiateTree() reads main_tree_to_execute from, and a document that
-  // did not parse has no root element at all.
+  // Load the document just appended to opened_documents, all or nothing.
+  // Through <include>, one call loads several documents: on failure restore
+  // the state from before the call and drop every document it opened, so none
+  // stays the "first opened document" instantiateTree() reads
+  // main_tree_to_execute from (issues #1266, #1282).
   void loadLastDocImpl(bool add_includes)
   {
     const auto doc_it = std::prev(opened_documents.end());
+    auto roots_before = tree_roots;
+    auto sources_before = tree_sources;
+    auto models_before = subtree_models;
+    auto path_before = current_path;
+    const int suffix_before = suffix_count;
     try
     {
       loadDocImpl(doc_it->get(), add_includes);
     }
     catch(...)
     {
-      opened_documents.erase(doc_it);
+      tree_roots = std::move(roots_before);
+      tree_sources = std::move(sources_before);
+      subtree_models = std::move(models_before);
+      current_path = std::move(path_before);
+      suffix_count = suffix_before;
+      opened_documents.erase(doc_it, opened_documents.end());
       throw;
     }
   }
@@ -466,7 +476,8 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes)
 
     include_stack.push_back(std::move(include_key));
     next_doc->LoadFile(file_path.string().c_str());
-    loadLastDocImpl(add_includes);
+    // no rollback here: the top-level load call undoes the whole load
+    loadDocImpl(next_doc, add_includes);
     include_stack.pop_back();
 
     // reset current path to the previous value

tests/CMakeLists.txt

@@ -61,6 +61,7 @@ set(BT_TESTS
   gtest_plugin_issue953.cpp
   gtest_blackboard_thread_safety.cpp
   gtest_xml_null_subtree_id.cpp
+  gtest_xml_include_rollback.cpp
 
   script_parser_test.cpp
   test_helper.hpp

tests/gtest_duplicate_tree.cpp

@@ -1,10 +1,10 @@
 /* Copyright (C) 2026 - duplicate BehaviorTree ID registration tests */
 
+#include "test_helper.hpp"
+
 #include "behaviortree_cpp/bt_factory.h"
 
-#include <chrono>
 #include <filesystem>
-#include <fstream>
 #include <string>
 #include <tuple>
 
@@ -14,35 +14,6 @@ using namespace BT;
 
 namespace
 {
-class TempSubdir
-{
-public:
-  TempSubdir()
-  {
-    const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count();
-    path = std::filesystem::temp_directory_path() /
-           ("bt_duplicate_tree_" + std::to_string(stamp));
-    std::filesystem::create_directories(path);
-  }
-
-  ~TempSubdir()
-  {
-    std::error_code ec;
-    std::filesystem::remove_all(path, ec);
-  }
-
-  std::filesystem::path path;
-
-  TempSubdir(const TempSubdir&) = delete;
-  TempSubdir& operator=(const TempSubdir&) = delete;
-};
-
-void writeFile(const std::filesystem::path& p, const std::string& content)
-{
-  std::ofstream out(p.string());
-  out << content;
-}
-
 std::string minimalTreeXml(const std::string& tree_id, const std::string& node = "AlwaysS"
                                                                                  "uccess")
 {

tests/gtest_try_catch.cpp

@@ -14,6 +14,8 @@
 
 #include "behaviortree_cpp/bt_factory.h"
 
+#include <stdexcept>
+
 #include <gtest/gtest.h>
 
 using BT::NodeStatus;
@@ -341,6 +343,43 @@ TEST_F(TryCatchTest, CatchOnHalt_Enabled)
   ASSERT_EQ(catch_count, 1);  // Catch executed on halt
 }
 
+// Destroying a RUNNING tree halts it; a catch child that throws during that
+// halt must not escape Tree::~Tree() and call std::terminate (issue #1281).
+// Covers both a std::exception and any other thrown type.
+TEST_F(TryCatchTest, CatchOnHalt_ThrowInDestructorDoesNotTerminate)
+{
+  factory.registerSimpleCondition("AlwaysRunning",
+                                  [](BT::TreeNode&) { return NodeStatus::RUNNING; });
+  int catch_ticks = 0;
+  factory.registerSimpleAction("ThrowStd", [&catch_ticks](BT::TreeNode&) -> NodeStatus {
+    catch_ticks++;
+    throw std::runtime_error("thrown while halting");
+  });
+  factory.registerSimpleAction("ThrowInt", [&catch_ticks](BT::TreeNode&) -> NodeStatus {
+    catch_ticks++;
+    throw 42;
+  });
+
+  for(const std::string catch_node : { "ThrowStd", "ThrowInt" })
+  {
+    const std::string xml_text = R"(
+      <root BTCPP_format="4">
+         <BehaviorTree>
+            <TryCatch catch_on_halt="true">
+              <AlwaysRunning/>
+              <)" + catch_node + R"(/>
+            </TryCatch>
+         </BehaviorTree>
+      </root>)";
+
+    {
+      auto tree = factory.createTreeFromText(xml_text);
+      EXPECT_EQ(tree.tickOnce(), NodeStatus::RUNNING);
+    }
+  }
+  EXPECT_EQ(catch_ticks, 2);
+}
+
 TEST_F(TryCatchTest, CatchOnHalt_NotTriggeredWhenAlreadyInCatch)
 {
   int catch_ticks = 0;

tests/gtest_xml_include_rollback.cpp

@@ -0,0 +1,116 @@
+/* A load call rejected after an <include> must leave the parser as it was
+ * before the call (issue #1282). */
+
+#include "test_helper.hpp"
+
+#include "behaviortree_cpp/bt_factory.h"
+#include "behaviortree_cpp/xml_parsing.h"
+
+#include <string>
+#include <tuple>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+using namespace BT;
+using Names = std::vector<std::string>;
+
+namespace
+{
+std::string treeXml(const std::string& body)
+{
+  return R"(<root BTCPP_format="4">)" + body + "</root>";
+}
+
+std::string tree(const std::string& tree_id, const std::string& node)
+{
+  return R"(<BehaviorTree ID=")" + tree_id + R"("><)" + node + "/></BehaviorTree>";
+}
+
+// main.xml includes a valid sub.xml, then defines MainTree with `main_node`
+std::filesystem::path writeMainWithInclude(const TempSubdir& dir,
+                                           const std::string& main_node)
+{
+  writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess")));
+  const auto main_xml = dir.path / "main.xml";
+  writeFile(main_xml,
+            treeXml(R"(<include path="sub.xml"/>)" + tree("MainTree", main_node)));
+  return main_xml;
+}
+}  // namespace
+
+TEST(XMLParserTest, RejectedIncludeKeepsNoTree)
+{
+  TempSubdir dir;
+  const auto main_xml = writeMainWithInclude(dir, "NotARegisteredNode");
+
+  BehaviorTreeFactory factory;
+  XMLParser parser(factory);
+  EXPECT_THROW(parser.loadFromFile(main_xml), std::exception);
+  EXPECT_EQ(parser.registeredBehaviorTrees(), Names{});
+}
+
+TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree)
+{
+  TempSubdir dir;
+  writeFile(dir.path / "sub.xml", treeXml(tree("FromInclude", "AlwaysSuccess")));
+  const auto text = treeXml(R"(<include path=")" + (dir.path / "sub.xml").string() +
+                            R"("/>)" + tree("MainTree", "NotARegisteredNode"));
+
+  BehaviorTreeFactory factory;
+  XMLParser parser(factory);
+  EXPECT_THROW(parser.loadFromText(text), std::exception);
+  EXPECT_EQ(parser.registeredBehaviorTrees(), Names{});
+}
+
+TEST(XMLParserTest, AcceptedIncludeKeepsItsTree)
+{
+  TempSubdir dir;
+  const auto main_xml = writeMainWithInclude(dir, "AlwaysSuccess");
+
+  BehaviorTreeFactory factory;
+  XMLParser parser(factory);
+  parser.loadFromFile(main_xml);
+  EXPECT_EQ(parser.registeredBehaviorTrees(), (Names{ "FromInclude", "MainTree" }));
+}
+
+// The rejected load's documents must all be dropped: a leftover included
+// document would become the first one, and its main_tree_to_execute would be
+// picked for a later file that does not name its main tree.
+TEST(XMLParserTest, RejectedIncludeIsNotTheFirstDocument)
+{
+  TempSubdir dir;
+  writeFile(dir.path / "sub.xml",
+            R"(<root BTCPP_format="4" main_tree_to_execute="FromInclude">)" +
+                tree("FromInclude", "AlwaysSuccess") + "</root>");
+  const auto main_xml = dir.path / "main.xml";
+  writeFile(main_xml, treeXml(R"(<include path="sub.xml"/>)" +
+                              tree("MainTree", "NotARegisteredNode")));
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.registerBehaviorTreeFromFile(main_xml), std::exception);
+
+  // two trees and no main_tree_to_execute: ambiguous, must be rejected
+  const auto ambiguous =
+      treeXml(tree("A", "AlwaysSuccess") + tree("FromInclude", "AlwaysFailure"));
+  EXPECT_THROW(std::ignore = factory.createTreeFromText(ambiguous), RuntimeError);
+}
+
+// A tree redefined by a valid include of a rejected file must get its
+// previous definition back: the redefining document is erased.
+TEST(XMLParserTest, RejectedRedefinitionRestoresPreviousTree)
+{
+  TempSubdir dir;
+  writeFile(dir.path / "good.xml", treeXml(tree("Shared", "AlwaysSuccess")));
+  writeFile(dir.path / "redefine.xml", treeXml(tree("Shared", "AlwaysFailure")));
+  writeFile(dir.path / "main.xml", treeXml(R"(<include path="redefine.xml"/>)" +
+                                           tree("MainTree", "NotARegisteredNode")));
+
+  BehaviorTreeFactory factory;
+  factory.registerBehaviorTreeFromFile(dir.path / "good.xml");
+  EXPECT_THROW(factory.registerBehaviorTreeFromFile(dir.path / "main.xml"),
+               std::exception);
+
+  auto tree_instance = factory.createTree("Shared");
+  EXPECT_EQ(tree_instance.tickOnce(), NodeStatus::SUCCESS);
+}

tests/test_helper.hpp

@@ -3,8 +3,11 @@
 
 #include "behaviortree_cpp/bt_factory.h"
 
+#include <chrono>
 #include <cstdio>
 #include <cstdlib>
+#include <filesystem>
+#include <fstream>
 #include <string>
 
 #if !defined(_WIN32)
@@ -85,4 +88,36 @@ inline void RegisterTestTick(BT::BehaviorTreeFactory& factory,
   }
 }
 
+/// A fresh directory under the system temp dir, removed with its content at
+/// the end of the scope.
+class TempSubdir
+{
+public:
+  TempSubdir()
+  {
+    const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count();
+    path = std::filesystem::temp_directory_path() / ("bt_test_" + std::to_string(stamp));
+    std::filesystem::create_directories(path);
+  }
+
+  ~TempSubdir()
+  {
+    std::error_code ec;
+    std::filesystem::remove_all(path, ec);
+  }
+
+  std::filesystem::path path;
+
+  TempSubdir(const TempSubdir&) = delete;
+  TempSubdir& operator=(const TempSubdir&) = delete;
+  TempSubdir(TempSubdir&&) = delete;
+  TempSubdir& operator=(TempSubdir&&) = delete;
+};
+
+inline void writeFile(const std::filesystem::path& p, const std::string& content)
+{
+  std::ofstream out(p.string());
+  out << content;
+}
+
 #endif  // TEST_HELPER_HPP