We should investigate the return statuses of the recoveries. They will all return success, even if in fact they halt early due to a potential collision. Ex.

  if (!isCollisionFree(distance, cmd_vel.get(), pose2d)) {
    stopRobot();
    RCLCPP_WARN(node_->get_logger(), "Collision Ahead - Exiting BackUp");
    return Status::SUCCEEDED;
  }

I remember in the early days we accepted this but I wonder now if we should have it fail and figure out the necessary changes to the behavior tree to handle this properly (use of different or custom control nodes for recoveries).

Within the context of the BT, the state return SUCCESS or FAILURE probably doesn't matter (?) since its trying to do something to fix a bad situation. Even if it only partially moves or particularly spins, that could still successfully unblock the robot, so even a FAILURE is a SUCCESS, maybe. That's perhaps an argument for having the recovery BT nodes handle the returned action status and always return a certain value or use a control flow node that will not do "bad things" due to recovery failures.

This is a real problem though if its called outside the context of the behavior tree. If I'm calling backup from a controller, I really want to know if it backed-up properly. Same with others. Right now the logic for always succeed is in the action server for the recovery itself, not in the BT node or the condition node hosting recovery BT nodes. This is making is so that when I write a test to intentionally collide with something, I still get success even though it failed, making it impossible to test failure conditions of the recoveries.

My suggestion - and open for discussion - is that we:

• Properly return the success / failure state in the recovery plugins in the recovery server
• In the Behavior Tree, either mask these values on return to always be success, or modify the control flow node used so that a failure of a recovery doesn't negatively impact performance.

CC @shivaang12. You should still write that spinning test, but be aware that the return value will be true, but we should still exercise it. I'm adding a note for my failure test case in backup with a link to this ticket to update once this is resolved. For reference: #1774