[dundysm]

Description

Fixes #3852.

TrajectoryExecutionManager::execution_complete_ was a plain bool used as a stop/completion flag between stopExecution() and the execution worker. Some accesses hold execution_state_mutex_, while executeThread() / executePart() (and a few other sites) read it without that mutex. That is a C++ data race: locking only the writer does not synchronize unlocked readers, and a later join() does not repair earlier conflicting accesses.

This change makes execution_complete_ a std::atomic<bool>. Existing assignment and boolean checks continue to work; the mutex + execution_complete_condition_ protocol used by waitForExecution() / worker completion is unchanged, and stopExecution() still joins the worker the same way.

Why atomic instead of mutex-only: #3198 already tightened locking around trajectory cleanup / waitForRobotToStop(). Forcing every hot-path poll in executePart() under execution_state_mutex_ would change contention characteristics. std::atomic<bool> matches nearby planning code that uses atomics for cross-thread stop/preemption flags (for example PlanExecution::preempt_).

All call sites of execution_complete_ were audited; only the member type needed to change.

Checklist

• Required by CI: Code is auto formatted using clang-format (one-line type/include change)
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

Test plan

• Audited every execution_complete_ access in trajectory_execution_manager.cpp / header.
• Full moveit_ros_planning / TrajectoryExecutionManager package build and gtests were not run here (no ROS/MoveIt desktop on the verification host). Existing trajectory_execution_manager gtests remain disabled/commented in that package's CMakeLists.txt.
• Standalone ThreadSanitizer sanity check of the same locked-write / unlocked-read pattern: plain bool reports a data race; std::atomic<bool> is clean. This is a pattern-level check, not a TSAN run of the MoveIt translation unit.
• Please rely on MoveIt CI for the real package build.

Summary by CodeRabbit

• Bug Fixes
  • Improved the reliability of trajectory execution status handling when updates occur concurrently. Execution completion status is now handled in a way that better supports concurrent access, reducing the risk of inconsistent status reporting during trajectory execution.

[dundysm]

@rhaschke @christian-rauch @elyforl gentle ping on this fix for #3852; execution_complete_ is now std::atomic so stopExecution and the worker no longer race on a plain bool. happy to adjust if you'd rather keep the bool and take execution_state_mutex_ on every read instead.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d52a4004-cc76-4675-946c-79e7fe5b714b

📥 Commits

Reviewing files that changed from the base of the PR and between 8590108 and 4a9ba69.

📒 Files selected for processing (1)

• moveit_ros/planning/trajectory_execution_manager/include/moveit/trajectory_execution_manager/trajectory_execution_manager.hpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds <atomic> and changes the private execution_complete_ field from bool to std::atomic<bool>.

Changes

Trajectory execution synchronization

Layer / File(s)	Summary
Atomic completion flag moveit_ros/planning/trajectory_execution_manager/include/moveit/trajectory_execution_manager/trajectory_execution_manager.hpp	Adds the <atomic> header and changes execution_complete_ to std::atomic<bool>.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: nbbrooks

Merge Risk: ⚪ Minimal · up to 4a9ba

The atomic completion flag addresses the reported race without changing the existing wait and stop behavior. No concrete user-facing regression is indicated, so merge risk is minimal.

Architecture Summary

Architecture risk: 🔵 Low · up to 4a9ba

The change affects 1 system.

Changed systems: moveit_ros

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — moveit_ros (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in moveit_ros/planning/trajectory_execution_manager/include/moveit/trajectory_execution_manager/trajectory_execution_manager.hpp: Adds the <atomic> header.
• observed — Modified behavior in moveit_ros/planning/trajectory_execution_manager/include/moveit/trajectory_execution_manager/trajectory_execution_manager.hpp: Changes the private execution_complete_ field from bool to std::atomic<bool>.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main change: fixing the race on TrajectoryExecutionManager::execution_complete_.
Linked Issues check	✅ Passed	Issue [#3852] requires a consistent synchronization mechanism for all concurrent accesses to execution_complete_. The whole-PR diff adds <atomic> and changes the member to std::atomic<bool>. Exi…
Out of Scope Changes check	✅ Passed	The whole-PR diff contains only the <atomic> include and the execution_complete_ type change in TrajectoryExecutionManager. Both changes directly implement issue [#3852]. No unrelated source, in…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.91%. Comparing base (277a334) to head (4a9ba69).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3853      +/-   ##
==========================================
- Coverage   48.91%   48.91%   -0.00%     
==========================================
  Files         735      735              
  Lines       62684    62684              
  Branches     7617     7617              
==========================================
- Hits        30658    30657       -1     
- Misses      32026    32027       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nbbrooks]

Thank you!

[mergify]

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

[dundysm]

rebased onto current main (no conflicts; race fix unchanged, still just std::atomic<bool> for execution_complete_).

rolling-ci ServoRosFixture.testJointJog looks unrelated to this change: assertion was traj_count_ == traj_count_before after intentionally invalid joint jog sizes (7 joints / 6 and 8 cmds), but one extra trajectory still landed (49 -> 50). timing flake in the servo ros integration test, not trajectory execution manager. twist/pose passed. no code change beyond the rebase; relying on ci re-run. @nbbrooks

[dundysm]

follow-up: after the rebase push, github actions for this fork pr are waiting on maintainer approval (action_required on CI + format runs). @nbbrooks if you can approve the workflow runs that should kick rolling-ci again.