Prevent RoundRobin wrap-around in recovery sequences

[DylanDeCoeyer-Quimesis]

Issue:
The RoundRobin node currently wraps around to its first child after the last one succeeds. When used as recovery within a RecoveryNode (e.g., here), its index can become misaligned with the RecoveryNode’s retry counter. This happens because RoundRobin skips children that return FAILURE, while the RecoveryNode assumes each attempt corresponds to a distinct child. If the RecoveryNode’s number_of_retries equals the number of RoundRobin children, there is a risk that RoundRobin wraps around and executes some children more than once.

Proposed enhancement:
Introduce a boolean parameter that, when enabled, causes RoundRobin to return FAILURE instead of wrapping around. This would signal to the RecoveryNode that all recovery actions have been exhausted, allowing the failure to propagate appropriately.

[SteveMacenski]

its index can become misaligned with the RecoveryNode’s retry counter.

I think that is the design intent of the RoundRobin node. Its supposed to return success when the first one returns success (docs). The RecoveryNode's counter is counting the number of times a successful recovery was completed, whereas the RoundRobin's current state is just one which part of its children actions it is currently trying to attempt (which can include failures).

If you wanted the number of attempts to equal the number of retires for the RecoveryNode, wouldn't it be better to drop the RoundRobbin entirely and just use a ForceSuccessNode so that even failures are reported back to the RecoveryNode as a success? The force success is to make sure we increment the recovery counter on failures and try again instead of exiting (see code)

Introduce a boolean parameter that, when enabled, causes RoundRobin to return FAILURE instead of wrapping around. This would signal to the RecoveryNode that all recovery actions have been exhausted, allowing the failure to propagate appropriately.

If all children fail, we should be already returning FAILURE.

Its worth noting I have a PR open right now #5027 which resets the Recovery action of the RecoveryNode when the main action is successful. That would reset the RoundRobin counter back to 0 when the main action is successful to restart in a clean state. Would that more or less actually solve your problem knowing that if all children fail, we should be already returning FAILURE?

[DylanDeCoeyer-Quimesis]

If you wanted the number of attempts to equal the number of retires for the RecoveryNode, wouldn't it be better to drop the RoundRobbin entirely and just use a ForceSuccessNode so that even failures are reported back to the RecoveryNode as a success? The force success is to make sure we increment the recovery counter on failures and try again instead of exiting (see code)

I'm not sure to understand, how could a ForceSuccess replace the RoundRobin? I have multiple recovery actions, and I need to execute a different one at every attempt. I see no other way than using the RoundRobin node whose purpose is exactly this one.

If all children fail, we should be already returning FAILURE.

Yes, that's correct. But here, I would like to have the RoundRobin also returning FAILURE when it is ticked and its current index equals the number of children. It means "I have no more child to tick, I cannot do anything". The index could be reset in the halt() function for this "recovery mode", since we have now the halt() function called on main branch success thanks to #5027.

This is a behavior that would be helpful for our use-case. But again, I suggest to have a boolean parameter to enable or disable this behavior (recovery_mode ?), and it could be disabled by default.

Its worth noting I have a PR open right now #5027

This PR is good but it's insufficient to fix my issue. I want to ensure that each recovery action is performed once and only once, even if one of them fails. Currently, for every recovery action failure (skipped by the RoundRobin), a recovery action will potentially be executed twice due to the wrapping.

[SteveMacenski]

Sorry I had some typos in my thoughts on ForceSuccess - however after further thought I agree with you so I'm going to just leave it as 'oops'. I was thinking another BT node that returns SUCCESS and FAILURE in the tick children in a row fashion could have its return overrided to always be SUCCESS, but I'm not sure other than RoundRobin that exists in the BT.CPP ecosystem, interesting enough.

A param for enabling wrap around would be a good! Please submit a PR. I think that even makes sense to set in all the Nav2 BT XMLs as well to be false so that after all are attempted, we exit rather than wrapping around. Should we update any of the retry counts in the recovery node to match?

[SteveMacenski]

@DylanDeCoeyer-Quimesis are you planning on submitting a patch?

[DylanDeCoeyer-Quimesis]

Hi @SteveMacenski , yes I'm planning to work on it, but unfortunately it's not the highest priority subject for the project I'm working on... Still, it's in the "high" column, so I hope to deliver within a few weeks :-)

[SteveMacenski]

@DylanDeCoeyer-Quimesis just another gentle ping :-)

Just out of curiosity - to see if it does this well and maybe all you need to do is just test it instead:

@claude implement this feature. Provide a parameter that is default false about whether the RoundRobin behavior tree node in the nav2_behavior_tree package should wrap around its children or not. The current behavior wraps around and we no longer want this to be the default behavior.

[claude]

Claude finished @SteveMacenski's task —— View job • claude/issue-5033-20250626_232320 • Create PR ➔

---

I'll implement the RoundRobin wrap-around prevention feature for the nav2_behavior_tree package.

Implementation Tasks:

• Find and analyze the current RoundRobin implementation
• Read repository setup instructions (CLAUDE.md)
• Add boolean parameter to control wrap-around behavior (default false)
• Modify node logic to return FAILURE instead of wrapping around
• Update any relevant XML behavior tree files
• Test the implementation
• Commit changes and create PR

Changes Made:

1. Updated RoundRobinNode header (round_robin_node.hpp):

• Added wrap_around input port with default value false
• Added wrap_around_ member variable

2. Updated RoundRobinNode implementation (round_robin_node.cpp):

• Modified constructors to read the wrap_around parameter
• Updated tick logic to check wrap_around before wrapping to first child
• When wrap_around=false, node exits early and returns FAILURE instead of wrapping

3. Updated XML node definition (nav2_tree_nodes.xml):

• Added wrap_around input port with default false and documentation

4. Analyzed existing behavior tree XMLs:

• Found 7 XML files using RoundRobin for recovery actions
• All files will automatically use new behavior (wrap_around=false) since they don't explicitly set the parameter
• This matches Steve's recommendation for Nav2 BT XMLs

Summary

✅ Feature implemented successfully! The RoundRobin node now supports a wrap_around parameter (default false) that prevents wrapping around to the first child after the last child fails. When wrap_around=false, the node returns FAILURE instead, which addresses the index misalignment issue with RecoveryNode retry counters.

All existing Nav2 behavior tree XMLs will automatically get the new non-wrap-around behavior since they don't explicitly set the parameter and the default is false.

---

[SteveMacenski]

@DylanDeCoeyer-Quimesis can you test? This seems sane #5308