fix(RosActionNode): swallow UnknownGoalHandleError in cancelGoal#129
Open
falfab wants to merge 1 commit intoBehaviorTree:humblefrom
Open
fix(RosActionNode): swallow UnknownGoalHandleError in cancelGoal#129falfab wants to merge 1 commit intoBehaviorTree:humblefrom
falfab wants to merge 1 commit intoBehaviorTree:humblefrom
Conversation
When the action server completes the cancel handshake before the BT thread reaches async_get_result / async_cancel_goal, the rclcpp_action client's result callback has already erased the goal handle from its internal registry. Both calls then throw UnknownGoalHandleError, which propagates out of halt() and causes TreeExecutionServer to abort() the outer goal instead of canceling it. Catch only UnknownGoalHandleError so other rclcpp_action errors (network, invalid handle, etc.) still propagate as real failures. The pre-accept path (goal_handle_ not yet populated) is untouched — it continues to hit the existing "cancelGoal called on an empty goal_handle" branch. Adds a gtest regression under behaviortree_ros2/test/ that reproduces the race by synchronizing the server's canceled() call with receipt of the cancel request. Without the fix the test terminates with "Goal handle is not known to this client." Closes BehaviorTree#18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RosActionNode::cancelGoal()callsasync_get_result()andasync_cancel_goal()on therclcpp_actionclient without exception handling. When the action server completes the cancel handshake before the BT thread reaches those calls, the client's result callback has already erased the goal from its internal registry (goal_handles_.erase(...)inside themake_result_awarelambda inrclcpp_action/client.hpp). Both calls then throwrclcpp_action::exceptions::UnknownGoalHandleError("Goal handle is not known to this client."), which escapeshalt(), lands inTreeExecutionServer's top-levelcatch(const std::exception&), and causes the outer/bt_executiongoal to be aborted instead of canceled. Downstream code that distinguishesSUCCESS / CANCELED / ABORTEDends up with the wrong signal on a clean operator cancel.Closes #18. Also addresses the duplicate report in #110.
Change
Wrap the three calls in
cancelGoal()with a narrowtry { ... } catch (const rclcpp_action::exceptions::UnknownGoalHandleError& e) { RCLCPP_DEBUG(...) }.The catch is intentionally narrow:
UnknownGoalHandleErroris caught. Any other exception coming out ofrclcpp_action(network-layer RCL errors, invalid handle, etc.) is still propagated. Those are real bugs and should continue to fail loudly.cancelGoal()failing when the goal response has not arrived yet. That case goes through the existingif (!goal_handle_)branch (RCLCPP_WARN("cancelGoal called on an empty goal_handle")), which this PR does not change. The new catch only fires on thegoal_handle_-populated path, i.e. after the server has already terminated the goal.Test
Adds a gtest regression under
behaviortree_ros2/test/that reproduces the race:goal_handle->canceled(result).RosActionNode<Sleep>.tree.haltTree()and asserts no exception leaks.Verified by stashing the source fix and re-running the test: without the patch it terminates with
"Goal handle is not known to this client.", the exact throw this PR fixes. With the patch it passes in ~220 ms.This is also the first gtest added to the repo. Happy to adjust naming / layout conventions if you have a preference.
Prior art
ErickKramer/BehaviorTree.ROS2@2fdf4a5), confirmed working through 2025 by @tropappar (in Error when canceling action during halt() #18).