[dvadovsz]

[written by AI]

Refs PickNikRobotics/moveit_pro#20169

Brings in the Finally changes made in review on the upstream PR, BehaviorTree#1229. The upstream reply that explains them is at BehaviorTree#1229 (comment).

• The halt loop now runs only when halt() is called from the thread that last ticked the node. A halt from another thread, such as the Timeout decorator's timer, gives cleanup one tick and prints a note to stderr. Before, Timeout ran the whole cleanup on its timer thread while holding timeout_mutex_, and the tick thread waited on that mutex. A Finally nested inside such a halt also gets one tick.
• A main that is SKIPPED on its first tick did nothing, so cleanup no longer runs and the node returns SKIPPED. A main skipped after it returned RUNNING, for example by _while, still runs cleanup.
• halt_timeout_msec="0" gives cleanup one tick on halt.
• tick_thread_ is atomic, and two helpers are noexcept.

One difference from upstream is deliberate. Upstream now defaults halt_timeout_msec to 0, while this fork keeps 10000, because MoveIt Pro 10.2 shipped with it and documents it. ObjectiveServer::runTree ticks and halts on the same thread, so a Stop still runs cleanup to completion, as before.

For MoveIt Pro, a follow-up is needed. The Finally description in src/behavior/config/standard_tree_nodes_model.xml still says cleanup runs when the main branch returned SKIPPED. Its halt_timeout_msec tooltip could also say that 0 gives one tick and that a Timeout above Finally gives cleanup only one tick. That belongs with the moveit_pro bump to this fork.

main-picknik already has the ControlNode::haltChild fix. This adds its regression test, tests/gtest_halt_child.cpp. Upstream has that fix as a separate PR, BehaviorTree#1273.

Tests: gtest_finally.cpp now has 46 cases, covering the default loop, the zero timeout, halts from another thread and from Timeout (including a nested Finally), SKIPPED before and after RUNNING, and a finished node ticked again without a reset. I checked that the nested Timeout case and the re-tick case fail without their fixes. behaviortree_cpp_picknik_test passes all 590 tests locally (GCC, Release), and the Finally tests passed 10 repeated runs. On the upstream branch, ThreadSanitizer reported no races and ASan/UBSan passed per test. pre-commit passes. picknik:code-reviewer ran on this branch and its findings are applied.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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

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