[dvadovsz]

[written by AI]

Refs PickNikRobotics/moveit_pro#20169

Replaces #43. This is the same change, plus three commits from the review gate, pushed to a branch on this repo instead of a personal fork.

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. If cleanup is still RUNNING after that tick, it 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.
• A halt_timeout_msec of 10 or less gives cleanup one tick on halt. halt() sets the deadline before the first tick and starts no tick past it, and the tick period is 10 ms.
• 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.

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.

Commits

• d9dc267 brings in the upstream review changes. It is unchanged from fix: Address upstream review of the Finally node #43.
• 85c780a corrects the docs. The port description said "below 10" and the class doc said the loop runs "above 0", but any value up to 10 gives one tick. Upstream's merged text already says "10 or less". The commit also fixes the matching CHANGELOG line and the tick_thread_ comment, and adds HaltTimeoutAtTickPeriod_CleanupGetsOneTick to pin the boundary.
• 95b756a adds TimeoutInsideMain_AsyncCleanupFinishes. With the Timeout inside main, the timer halts only main, and cleanup finishes over several ticks. That is the pattern to recommend instead of a Timeout above Finally.

Follow-ups for the moveit_pro bump to this fork

The bump moves BTCPP_PICKNIK_VERSION from 4.9.0-5noble to a new Debian revision. compatibility-bot found no ABI break. The FinallyNode layout changes are private, and only libbehaviortree_cpp instantiates the class. That bump PR needs the following:

• A release note for each behavior change, with team sign-off. These changes are:
  • A Timeout above Finally now gives cleanup one tick and then halts it. In 10.2 the cleanup ran to completion. The robot ends up stopped, because an async cleanup motion is cancelled. Cleanup that restores state other than motion does not finish, though. Examples are a collision rule, an active controller, an open gripper or an attached object, and that state can carry into the next Objective. roboticist-bot rated this P1 for the bump.
  • When main is SKIPPED on its first tick, cleanup no longer runs.
• Updated Finally text in src/behavior/config/standard_tree_nodes_model.xml. Today it says cleanup runs after SKIPPED and that a Stop keeps cleanup running until it finishes. Its halt_timeout_msec tooltip should say that 10 or less gives one tick and that a Timeout above Finally gives one tick. It should also recommend putting the Timeout inside main.
• A load-time warning, in verifyXML or Objective registration, for a Finally that has a Timeout ancestor. The message should name the pattern of putting the Timeout inside main. The stderr note only reaches the container log.

No Objective in moveit_pro or moveit_pro_example_ws uses Finally today.

Tests

Run on 95b756a:

• behaviortree_cpp_picknik_test passes all 592 tests (GCC, Release).
• FinallyTest.* and the HaltChild tests passed 50 repeated runs.
• The two new tests passed 200 and 100 repeated runs.
• Merged with the current main-picknik, the tree is the same as 95b756a. Under ThreadSanitizer, the Finally, HaltChild and Timeout tests ran 10 times and the full suite ran once, with no warnings. TSan runs 591 tests, because BlackboardTest.AnyPtrLocked is compiled out under USE_SANITIZE_THREAD.
• pre-commit passes, including clang-format and clang-tidy.

On #43, I checked that the nested Timeout case and the re-tick case fail without their fixes.

Claude agent checks

Reviewed on 95b756a, base main-picknik 183a390, merge base 1cc7638. The two commits between the merge base and the base, #45 and its revert #46, net to zero. Each reviewer role ran as a T3 child task in pi. Later commits were re-reviewed by every role whose domain they touched.

• code-reviewer: no P0 or P1. Its P2s on the boundary wording, the stderr-note wording, the stale code comment and the new tests are applied. Its other P2s were about the fix: Address upstream review of the Finally node #43 body, and this description addresses them.
• documentation-bot: no documentation impact on this repo. The moveit_pro Finally text goes stale at the bump, as listed above.
• licensing-privacy-bot: SKIPPED. No vendored code, dependency, or data flow changes.
• platform-architect-bot: no P0 or P1. The tick_thread_ comment fix is applied. Using >= in the deadline check is deferred, because upstream's merged code also uses >, the change would add fork difference, and Linux never hits the case. The release-note P2s are listed above.
• roboticist-bot: no P0. Its P1 belongs to the moveit_pro bump and is listed above. The test for the recommended Timeout-inside-main pattern is added. A main-halt counter in that test is deferred, because the Timeout tests already cover a timer halting its child.
• frontend-noah-bot: SKIPPED. No frontend.
• security-auditor: SKIPPED. No REST, bridge, LLM, Electron, or container paths.
• compatibility-bot: no breaking change, and a Debian revision bump is enough. Its two Needs-discussion items and its contract-drift item are listed under follow-ups.
• sonar-bot: predicts cpp:S5421 (CRITICAL) on thread_local bool tls_in_foreign_halt in finally_node.cpp, and cpp:S5952 (MINOR) on the forwarding constructor in gtest_halt_child.cpp. Both are deferred. Both lines are identical to upstream's merged code, and this fork's Sonar workflow is disabled (sonarcube.yml.bkp), so changing them would add fork difference for a gate that does not run. sonar-bot found no suppressions.
• test-runner: passed, with the results listed under Tests.

[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