[Quchaosheng]

ReactiveSequence and ReactiveFallback halt the siblings of a child only after that child returned RUNNING. When the new child starts an asynchronous action in its onStart(), the action of the branch that is about to be preempted is still alive, so for a short time two actions run concurrently. With the ROS action wrappers that means that a new goal is sent before the previous one is cancelled, which is the race described in issue #1031.

Let an asynchronous node stop the branches that are still RUNNING right before it transitions out of IDLE, walking up to the root. Parallel is the only control node designed to keep several children running at the same time, so it stops the walk.

To know the ancestors, every node now stores the parent that the parser sets when the children are added to the tree.

This follows the direction invited in #1031 for reworking #93, without the propagateHalt coupling: the async node asks its ancestors to release the other branches, instead of every control node having to know about asynchronous children.

Reproduced with ReactiveSequence[ ReactiveFallback[BatteryOk, GotoCharging], GotoKitchen ]:

before: GotoKitchen:onStart ... GotoCharging:onStart GotoKitchen:onHalted
max concurrent actions = 2
after: GotoKitchen:onHalted GotoCharging:onStart
max concurrent actions = 1

The new test fails without the fix (max concurrent actions 2, halt after start) and passes with it. All 523 tests pass.

Fixes #1031

[Quchaosheng]

A gentle first ping on this one — it has not had a review yet.

The race is the one from #1031, and the diff is the direction that issue invited. Reproduced here with ReactiveSequence[ ReactiveFallback[BatteryOk, GotoCharging], GotoKitchen ], where the two actions overlap for a moment before the halt lands:

before: GotoKitchen:onStart ... GotoCharging:onStart  GotoKitchen:onHalted
after:  GotoKitchen:onHalted  GotoCharging:onStart

The test in the PR fails without the fix and passes with it; the full suite is 523/523. CI is green on the current head as well.

Nothing here needs an answer from me — it needs a reviewer's time. If the parent-pointer approach is not the shape you want for this, say so and I will rework it.

[Quchaosheng]

I reran the full suite from this branch on current master, not just the new test:

cmake -S . -B build -DBTCPP_SQLITE_LOGGING=ON -DBUILD_TESTING=ON
cmake --build build -j4
./build/tests/behaviortree_cpp_test

523 tests from 61 test suites ran. PASSED 523.

The branch is based on ca4edbf and still merges cleanly into today's master; nothing since then touched the eight files in the diff.

@facontidavide the design question is whether a node may walk up to the root and halt siblings before its own onStart(). The alternative in #93 was to have every control node know about asynchronous children (propagateHalt); this keeps that knowledge in the action node instead. Parallel and ParallelAll opt out so their branches still run concurrently.

If you prefer the other shape, or want this limited to StatefulActionNode instead of also ThreadedAction and CoroActionNode, tell me which and I will change it. If the direction is fine, a review is all it needs.

[facontidavide]

Thanks @Quchaosheng for taking on #1031. The bug is real. In ReactiveSequence[ReactiveFallback[BatteryOk, GotoCharging], GotoKitchen], GotoCharging::onStart() runs before GotoKitchen is halted, so for one tick two async actions are alive at once. Your test reproduces it: it fails on master and passes with this branch. I can't merge this version, though, for three reasons:

1. Running actions are restarted on every tick. The halt happens on the IDLE → onStart() path, before anyone knows whether onStart() will return RUNNING. Take ReactiveSequence[QuickCheck, LongAction], where QuickCheck is a StatefulActionNode whose onStart() returns SUCCESS right away:

• Tick N: LongAction returns RUNNING, and ReactiveSequence halts the other children (reactive_sequence.cpp:47-52). QuickCheck is IDLE again.
• Tick N+1: QuickCheck goes IDLE → onStart() → haltRunningSiblings() halts LongAction. QuickCheck returns SUCCESS, and LongAction is started again from scratch.

So LongAction is cancelled and restarted on every tick, where master just calls onRunning(). ReactiveFallback with a child that fails immediately behaves the same way. This pattern is very common, for example a quick check implemented as a stateful action guarding a navigation action.

2. ABI break. virtual bool allowsConcurrentChildren() adds a vtable entry to TreeNode, which changes the layout of every node class. 4.x has to stay ABI-compatible, so no new virtuals in TreeNode.

3. Halts reach beyond the reactive parent. The walk halts RUNNING siblings at every ancestor up to the root, across SubTree boundaries. Only Parallel and ParallelAll opt out, so a user's control node that keeps several children running (for example nav2's PipelineSequence) would now get them halted. The halts also come from outside the ancestor's own tick, so its bookkeeping (e.g. running_child_) goes stale. And only StatefulActionNode, ThreadedAction and CoroActionNode get the hook, so the ROS action wrappers mentioned in #1031 wouldn't benefit.

A version I could accept would:

• not halt anything until the new child actually returns RUNNING, or be opt-in (off by default) on the reactive node;
• add no new virtual to TreeNode;
• include a test for the QuickCheck case above, asserting that LongAction is not restarted.

Happy to discuss the design here before you rework it.

[Quchaosheng]

Thanks Davide — all three are right, and I checked each one against a build of the
branch rather than against the argument. Then I prototyped the version you described
and it does fix #1031 without the three problems. Design first, as you asked.

What I confirmed on the branch

1. The restart is real, and not limited to "on every tick" of a stateful action.
ReactiveSequence[QuickCheck, LongAction], QuickCheck::onStart() returning SUCCESS,
five ticks: LongAction::onStart is called 5 times and onHalted 4 times, against
one start and zero halts on master. The pair is haltRunningSiblings() at
src/action_node.cpp:178 (the StatefulActionNode::tick() IDLE branch, before anyone
knows what onStart() will return) plus the halt loop at
src/controls/reactive_sequence.cpp:47-52. CoroActionNode reaches the same place by a
different route: its hook sits in the _p->coro == nullptr branch, which is true again
on every tick after the coroutine has finished, so a coroutine that returns SUCCESS
gives LongAction 4 starts and 3 halts in 4 ticks.

2. ABI. TreeNode's vtable is 8 entries before and 9 after, with
allowsConcurrentChildren at slot 48, in front of tick()/halt(). sizeof(TreeNode)
stays 16 because the new parent pointer went behind the existing PImpl, but the
vtable shifts every subsequent slot, so any node class compiled against 4.x headers
mismatches. It is not merely theoretical: the same consumer source built against the
old headers and linked to this branch's library aborts on the first tick in
TreeNode::setStatus() — the call lands on the wrong slot and the node tries to set
itself IDLE.

3. Reach. I copied nav2's PipelineSequence verbatim (same algorithm, same
last_child_ticked_) under ReactiveSequence[QuickTick, PipelineSequence[NavA, NavB]].
With QuickTick::onStart() returning SUCCESS, every tick from the second one logs
PipelineSequence::halt() from outside its own tick, then NavA::onHalted, then
QuickTick::onStart, then NavA::onStart from scratch. master logs QuickTick::onStart
and NavA::onRunning, nothing else. The halt() that resets last_child_ticked_ is
the visible half; the running_child_ staleness you mentioned is the same defect one
level up, because the halt comes from outside the ancestor's tick and the ancestor's
record of what it is running is not consulted. And you are right that only the three
BT:: action bases are hooked — nav2's BtActionNode derives from ActionNodeBase and
overrides tick() itself (bt_action_node.hpp:263), so the ROS wrappers in #1031 get
nothing from this branch.

A version I think you can merge

Three changes to your sketch. The substantive one is where the halt is triggered.

Trigger it in TreeNode::executeTick(), not in StatefulActionNode::tick().
That is the one place every action passes through, including ActionNodeBase
subclasses that override tick() themselves like nav2's. Condition:

if(_p->status == NodeStatus::IDLE && type() == NodeType::ACTION)
{
  preemptRunningSibling();
}

Don't walk to the root — stop at the nearest reactive ancestor, and halt through it.
The walk stops at Parallel/ParallelAll/SubTreeNode, and at the first
ReactiveSequence/ReactiveFallback. Two rules inside that ancestor:

• if that node already has a running child and this action is not inside it (or there
  is no reactive ancestor between the two), the start is a sibling re-evaluation, not
  a new branch: do nothing. This is what makes the QuickCheck case pass — the
  second tick finds running_child_ == 1, LongAction is inside it, so nothing is
  halted.
• otherwise it halts through rs->haltRunningChild(child), which is a member of the
  reactive node: it halts the child and clears running_child_. Halting from the
  outside with haltChild() is what leaves the bookkeeping stale, so the halt has to
  go through the node that owns the state.

Opt-in per node, and store the flag in the PImpl. setPreemptionEnabled(true) on
the reactive node; no new virtual anywhere. The flag lives next to the parent
pointer, so sizeof(TreeNode) and the vtable are both unchanged — verified 8 entries
and 16 bytes after the change. A process-wide static would have been one line shorter
but would make two trees in the same process share a policy, so I put it on the node.

Net effect on the halting scope: halts only ever come from the reactive node itself,
only for its own direct running child, never across a SubTree, never under a
Parallel, and never from an ancestor that is merely on the path.

Tests, including the one you asked for

Both in tests/gtest_reactive_backchaining.cpp, and I checked that each one fails
where it should rather than just passing on the new code:

test	master	current branch	new design
HaltBranchBeforeStartingAnotherAsyncAction (#1031)	FAIL (max_alive=2)	PASS	PASS
QuickCheckDoesNotRestartRunningSibling	PASS	FAIL (LongAction:start 5×)	PASS

QuickCheckDoesNotRestartRunningSibling is your case:
ReactiveSequence[QuickCheck, LongAction], InstantCheckAction::onStart() returns
SUCCESS, five ticks, EXPECT_EQ(countOf("LongAction:start"), 1) and
EXPECT_EQ(countOf("LongAction:halted"), 0). It passes on master precisely because that
is the behaviour you want preserved.

The full suite is green with the change in place: 575 tests, 573 passed, 2 skipped —
and the 2 skipped are the same two that master skips. I also checked a nav2-shaped
ActionNodeBase subclass (send goal in tick(), cancel in halt()): max_running
goes from 2 to 1 with the opt-in on, and cancel lands before the new send_goal.

What this does not fix

If the previous branch completes on its own while the next one starts — a plain
Sequence[Quick, A] sibling inside a ReactiveSequence, where A returns SUCCESS and
is later restarted — there is no running child to halt at that moment, so master and
the new design both reach max_running=3. The current branch reduces it to 2 only
because it halts more eagerly, which is the same eagerness that breaks QuickCheck. I
would rather leave that out than trade one for the other; #1031 as filed is the
preemption case, and it is fixed.

How do you want it

I have this working as a prototype on current master: about 120 lines of code across
tree_node.{h,cpp}, control_node.cpp, decorator_node.cpp,
reactive_{sequence,fallback}.{h,cpp}, plus the two tests. Nothing is pushed — tell
me which of these you want and I will turn it into the branch:

1. Opt-in per node, as above. Default behaviour of every existing tree unchanged;
   users who want Using ReactiveSequence/ReactiveFallback with multiple async children can cause race conditions #1031 fixed call setPreemptionEnabled(true) on the reactive node.
2. On by default. Closer to what Using ReactiveSequence/ReactiveFallback with multiple async children can cause race conditions #1031 asked for, but it changes semantics for
   ordinary trees: anything shaped like ReactiveSequence[Sequence[Quick, A], B] gets
   A restarted. I would not do that in 4.x.
3. Opt-in, and additionally expose it in XML as an attribute on
   ReactiveSequence/ReactiveFallback so it can be turned on per tree instead of in
   code. More surface, but avoids every consumer writing a visitor over subtree->nodes
   the way my test helper does.

My preference is 1, with 3 as a follow-up if people ask for it. The one thing I do not
have an answer for is naming — preemptRunningSibling describes the mechanism, not
what the user gets; if you have a name you prefer for the option, I will use it.

Thanks for the detailed review — the QuickCheck case in particular is the one I should
have tested before opening this.

Chaosheng

[facontidavide]

Thanks @Quchaosheng, this is a very useful analysis, and you confirmed each point with a real build.

Please rebase onto master. #1258 added three tests that pin the behavior master must keep: your QuickCheck case for both ReactiveSequence and ReactiveFallback (including a CoroActionNode check), and a PipelineSequence-like control whose concurrent children must not be halted. They pass on master and fail on the current branch with your numbers (5 starts, 4 halts). Any version of the fix has to keep them green.

On the design: I agree with your option 1 (off by default), but I don't think the "sibling re-evaluation vs new branch" rule can work. Before onStart() runs, a check that will return SUCCESS and a new action that will keep running look identical. The rule tells them apart by tree shape, and that only works for the shape in the test:

• ReactiveSequence[Fallback[BatteryOk, GotoCharging], GotoKitchen] with a plain Fallback looks exactly like QuickCheck, so nothing is halted and the race stays.
• If the walk stops at the nearest reactive ancestor, then in your battery tree that is the inner ReactiveFallback, which has no running child, so it can't reach GotoKitchen. I guess the prototype works differently from the description?
• An action inside a SubTree is never covered if the walk stops at SubTreeNode. SubTree is a blackboard boundary, not a concurrency one.
• ThreadedAction::executeTick() doesn't call TreeNode::executeTick(), so the hook misses it.
• In a nested reactive node, an action that returns SUCCESS immediately counts as a "new branch", which brings the QuickCheck restart back.

Suggestion: let the tree author mark the switch point explicitly, for example with a new decorator around the action that starts the new branch:

<ReactiveSequence>
  <Fallback>
    <BatteryOk/>
    <Preempt><GotoCharging/></Preempt>
  </Fallback>
  <GotoKitchen/>
</ReactiveSequence>

On its first tick, before ticking its child, Preempt halts the RUNNING child of each reactive ancestor it is not inside, through a member of that reactive node so running_child_ stays correct. Children of a Parallel are skipped, but the walk continues above it. This answers the "is this a new branch" question at the only place that knows it, the tree author. It works for every action type (ThreadedAction, nav2's BtActionNode), and it changes nothing for trees that don't use it, so no flag on the reactive nodes is needed. The name is open; Preempt is just a placeholder.

For 4.x the parent pointer and the halt helpers should stay internal: state in the existing PImpl, non-virtual members, no new virtuals.

Tests I'd like to see with it, in addition to #1258:

• the battery tree with a plain Fallback and with a ReactiveFallback
• the action inside a SubTree
• a ThreadedAction
• a Parallel inside the new branch, whose children must keep running together

One caveat for the ROS use case in #1031: halting only sends the cancel; it doesn't wait for the remote action to stop.

[Quchaosheng]

You are right on all counts, and I reproduced the numbers you quote before touching anything: on the previous head, the three tests from #1258 fail with Follow:start 5 and Follow:halted 4, exactly as you said. I also checked your five objections against the code, and they hold — in particular the ThreadedAction one, whose executeTick() is final and never runs the body of TreeNode::executeTick(), so a hook there simply cannot see it.

I have replaced the approach with the one you suggested and pushed 5d7c6c3. The branch is already up to date with master (0 commits behind), so no rebase was needed beyond the merge that is already in it.

What it does now

A new decorator, Preempt (placeholder name, as you said). It wraps the action that starts the new branch:

<ReactiveSequence>
  <ReactiveFallback>
    <BatteryOk/>
    <Preempt>
      <GotoCharging/>
    </Preempt>
  </ReactiveFallback>
  <GotoKitchen/>
</ReactiveSequence>

On its first tick, before ticking its child, it walks up the tree and halts the RUNNING child of every reactive ancestor it is not itself inside. The "not inside" part is what makes it safe: when the ancestor's running child is the branch we came from, halting it would halt ourselves, so it is skipped. Children of a Parallel are not touched — that node is allowed to keep several of them RUNNING — while the walk continues above it. A SubTree needs no special case: the parent chain ends at the subtree root, and the reactive node above the SubTree is reached because SubTree is a node like any other.

ReactiveSequence and ReactiveFallback now share a ReactiveControlNode base that owns running_child_ and exposes runningChild() / haltRunningChild(), both non-virtual, so the index stays consistent with the child that is actually RUNNING rather than being reset behind the node's back.

What is gone

The old mechanism is removed entirely: the haltRunningSiblings() call in StatefulActionNode, CoroActionNode and ThreadedAction, the method itself, and the allowsConcurrentChildren() virtual with its two overrides. For 4.x the parent pointer stays in the existing PImpl, and no new virtuals are added anywhere.

Tests

Added tests/gtest_preempt.cpp with the four cases you asked for:

• the battery tree with a plain Fallback and with a ReactiveFallback — both assert GotoKitchen:halted comes before GotoCharging:start, and that the reactive fallback does not halt it a second time
• the action inside a SubTree
• a ThreadedAction
• a Parallel inside the new branch, asserting both its children are still running afterwards and neither was halted

I also updated HaltBranchBeforeStartingAnotherAsyncAction in gtest_reactive_backchaining.cpp to use Preempt, since it was written against the removed automatic behaviour.

Full suite: 591 passed, 2 skipped, 0 failed. The three #1258 tests pass.

Two caveats

• Preempt is a placeholder name, as you said. Rename it freely before merge; I did not want to pick one unilaterally.
• Your point about Using ReactiveSequence/ReactiveFallback with multiple async children can cause race conditions #1031 stands unchanged: halting only sends the cancel, it does not wait for the remote action to stop. Nothing in this change addresses that.

[facontidavide]

CI is red

[Quchaosheng]

Both red checks are fixed in b231399.

pre-commit was formatting only: clang-format reorders the preempt.h include in behavior_tree.h and drops three stray blank lines.

build (ubuntu-22.04, tsan) was a real race, but in the new test rather than in the library. Tracker::events is written from the ThreadedAction worker thread (onStart/onHalted) while waitFor() polls hasEvent() on the main thread, so the vector could be reallocated under the reader. Tracker now holds a mutex around the vector and exposes count(), so the fixture no longer reaches into the list directly.

On the previous commit it reproduces 3/3 locally with setarch -R (exit 66); after the change it is 5/5 clean, and the whole Preempt and ReactiveBackchaining suites pass under tsan. No library code changed for either fix.