Skip to content

Clean up action goals on client disconnect - #1318

Open
grimgrimberg wants to merge 2 commits into
RobotWebTools:ros2from
grimgrimberg:fix/issue-1291-action-goal-cleanup
Open

grimgrimberg wants to merge 2 commits into
RobotWebTools:ros2from
grimgrimberg:fix/issue-1291-action-goal-cleanup

Conversation

@grimgrimberg

Copy link
Copy Markdown

Summary

  • add an optional cancel_on_disconnect flag to send_action_goal, defaulting to false for backward compatibility
  • clear per-client action handler state during Protocol.finish() and asynchronously cancel only opted-in goals
  • avoid the completion/disconnect race by swapping the handler map before cleanup
  • document and test the new protocol field and disconnect behavior

Testing

  • Ruff format and lint checks on changed Python files
  • fresh ROS 2 Lyrical build of rosbridge_test_msgs and rosbridge_library
  • complete rosbridge_library test run: 181 tests, 0 errors, 0 failures, 0 skipped
  • mypy: 63 files checked, 0 errors

All testing used local simulated ROS nodes only. No robot, vehicle, or external ROS target was contacted.

Fixes #1291

Copilot AI lite review requested due to automatic review settings September 8, 2026 10:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The current finish() implementation can race with concurrent dict mutation and can spawn an unbounded number of threads on disconnect, which can cause runtime errors and operational instability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses action goal lifecycle cleanup on client disconnect by adding an opt-in cancel_on_disconnect flag to send_action_goal, ensuring per-client action handler state is dropped during Protocol.finish(), and providing tests/documentation for the new behavior.

Changes:

  • Add optional cancel_on_disconnect (default false) to the send_action_goal operation and plumb it through to ActionClientHandler.
  • Implement SendActionGoal.finish() to drop per-client handler state and asynchronously cancel opted-in goals.
  • Extend protocol documentation and add unit tests for type validation and disconnect cleanup behavior.
File summaries
File Description
ROSBRIDGE_PROTOCOL.md Documents the new cancel_on_disconnect field for send_action_goal.
rosbridge_library/test/capabilities/test_action_capabilities.py Adds tests covering invalid cancel_on_disconnect types and finish() disconnect cleanup/cancel behavior.
rosbridge_library/src/rosbridge_library/internal/actions.py Adds cancel_on_disconnect to ActionClientHandler so the capability can decide disconnect-time cancellation.
rosbridge_library/src/rosbridge_library/capabilities/send_action_goal.py Adds message-field validation, passes cancel_on_disconnect through, and implements finish() cleanup/cancel logic.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +160 to +166
def finish(self) -> None:
client_handlers = self.client_handler_list
self.client_handler_list = {}

for client_handler in client_handlers.values():
if client_handler.cancel_on_disconnect and client_handler.send_goal_helper is not None:
Thread(target=client_handler.send_goal_helper.cancel_goal, daemon=True).start()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 5dfaaef. Handler add/remove/finish operations now share a lock, finish prevents late re-registration, and opted-in goals are cancelled sequentially by one background worker. Added regression tests for sequential cancellation and post-finish registration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SendActionGoal leaks in-flight goals on client disconnect — no finish() cleanup hook

2 participants