feat: Add Finally control node for guaranteed cleanup

[dv-picknik]

[written by AI]

Adds a Finally control node with two children, main and cleanup. It ticks main, then ticks cleanup, which gives Objectives try/finally semantics for resetting scene state such as collision rules.

The same change is proposed upstream in BehaviorTree#1229, for upstream issue BehaviorTree#1228. This PR does not wait for it, since upstream may decline the node. If upstream merges it, the next upstream port brings it in and we can drop the fork copy.

<Finally>
  <Sequence>
    <SetCollisionRule .../>
    <ComplexOperation/>
  </Sequence>
  <SetCollisionRule .../>
</Finally>

• After main returns SUCCESS, FAILURE or SKIPPED, cleanup runs and the node returns main's status. If cleanup fails, the node returns FAILURE, because a failed cleanup can leave the scene dirty.
• If main throws, the node halts main, runs cleanup, and then rethrows the exception.
• If the node is halted while main is RUNNING, it halts main and ticks cleanup once, synchronously, on the calling thread. Halt-time cleanup has to be synchronous. A cleanup that returns RUNNING gets halted. A halt while cleanup itself is RUNNING halts cleanup.
• halt() never throws, because ~Tree() calls haltTree() and a throw there calls std::terminate. It prints exceptions and a cleanup FAILURE to stderr instead.
• ControlNode::haltChild now resets the child's status before rethrowing an exception from its halt(). Without that, the next reset would halt the child again and throw a second time.
• The XML loader rejects a Finally without exactly 2 children.

Related: PickNikRobotics/moveit_pro#20169, milestone 10.2.0. The first commit here is a cherry-pick of the upstream commit. The later fix commits are squashed into that upstream commit, and the Finally files, control_node.cpp and the tests are identical on both branches.

TryCatch, which arrived with the 4.9.0 port, doesn't cover this. It skips the catch child on SUCCESS and does not catch exceptions.

Differences from the issue text:

• The issue asked for a thrown exception to become FAILURE. The node rethrows it after cleanup instead. That matches finally in other languages, keeps the NodeExecutionError context for loggers, and stops a programming error such as a missing required port from being retried by a RetryUntilSuccessful as if it were an ordinary failure. The tradeoff is that an outer Fallback can't recover from it.
• I skipped the single-child decorator variant the issue mentions.

Validation: behaviortree_cpp_picknik_test passes all 561 tests locally (Release, GCC) after the rebase onto main-picknik, and upstream's behaviortree_cpp_test passes all 565 on the upstream branch. 31 FinallyTest cases cover sync and async main and cleanup, a halt in each phase, exceptions from main, from cleanup, and from main's own halt(), non-std exceptions, SKIPPED main and cleanup, retry after a throw, nested Finally, SubTree and ReactiveSequence parents, and child-count validation. I checked that the key tests fail without their fix. Removing the haltChild reset aborts the test binary with std::terminate.

Reviewers run: picknik:code-reviewer, picknik:platform-architect-bot, picknik:sonar-bot, and the CodeRabbit CLI, with findings applied. Deferred:

• Sonar cpp:S3656 on the protected gtest fixture members, to match upstream's gtest_try_catch.cpp.
• cpp:S2738 and cpp:S1181 on the catch-alls in halt(), which exist so ~Tree() can't terminate.
• License headers on the new files. About 20 upstream source files carry none either.

roboticist, frontend, security, documentation, licensing and compatibility bots: skipped, since no trigger paths are touched and the change is a pure addition.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Team
• Run ID: 8803fae0-fd3e-4c08-bc12-2de0dccef09f

📥 Commits

Reviewing files that changed from the base of the PR and between 5e1ce6f and c4c6e3b.

📒 Files selected for processing (10)

• CHANGELOG.rst
• CMakeLists.txt
• include/behaviortree_cpp/behavior_tree.h
• include/behaviortree_cpp/controls/finally_node.h
• src/bt_factory.cpp
• src/control_node.cpp
• src/controls/finally_node.cpp
• src/xml_parsing.cpp
• tests/CMakeLists.txt
• tests/gtest_finally.cpp

Included review availability: This review used your included allowance. 9 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour.

---

📝 Summary

Summary by CodeRabbit

• New Features
  • Added the Finally control node, which runs a cleanup action after its main action completes, fails, or throws. Exceptions from the main action are rethrown after cleanup; a cleanup failure takes precedence over the main result.
  • Cleanup also runs when a running Finally node is halted. The node requires exactly two children: a main action and a cleanup action.
• Bug Fixes
  • A child that throws while being halted no longer remains marked as running.

Walkthrough

This change adds a built-in Finally control node with a main child and a cleanup child. The cleanup child runs after main-child completion or failure, and runs during a halt. The change also adds XML child-count validation and tests for status, exception, and halt behavior.

Changes

Finally control node

Layer / File(s)	Summary
Declare and integrate Finally include/behaviortree_cpp/controls/finally_node.h, src/bt_factory.cpp, CMakeLists.txt, include/behaviortree_cpp/behavior_tree.h, src/xml_parsing.cpp, CHANGELOG.rst	Declares FinallyNode, exposes it through the umbrella header, registers the built-in ID, and adds its implementation to the build. XML validation requires exactly two children.
Implement main and cleanup execution src/controls/finally_node.cpp, src/control_node.cpp	Runs the cleanup child after the main child completes or throws. A running cleanup keeps the node running. The node rethrows a saved main exception after cleanup; otherwise, cleanup failure returns FAILURE. Halt handling runs cleanup and logs suppressed exceptions. ControlNode::haltChild resets a child’s status before rethrowing a halt exception.
Validate Finally behavior tests/gtest_finally.cpp, tests/CMakeLists.txt	Adds tests for status propagation, cleanup execution, exceptions, halting, retries, nested trees, and invalid child counts.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to c4c6e

The Finally node is ready to merge after normal checks; no outstanding behavior issue was established.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore (reviewers only)

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Human Review Check	❌ Error	The PR adds a public BT::FinallyNode declaration under include/behaviortree_cpp/controls/finally_node.h, exposes it through the public behavior_tree.h umbrella header, and registers it as a buil…	This PR requires review by a requested human reviewer. After review, a non-author requested reviewer should override this pre-merge check.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The description explains the purpose, behavior, compatibility tradeoffs, tests, and validation for the change. It does not state whether the author followed the repository’s pre-commit, clang-tidy, or…

Full details: Human Review Check

Explanation

The PR adds a public BT::FinallyNode declaration under include/behaviortree_cpp/controls/finally_node.h, exposes it through the public behavior_tree.h umbrella header, and registers it as a built-in factory node. This is a public API change and meets an explicit failure condition.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Pre-merge checks failed. Please resolve the failing checks before merging.