Guard publishFeedback and execute for valid goal states#1466
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Action Server goal-handle API by preventing invalid state transitions and feedback publication in non-executing states (a known crash vector in underlying rcl), aligning behavior with upstream rclpy.
Changes:
- Guard
ServerGoalHandle.execute()to no-op when the goal is already executing, inactive (terminal), or destroyed. - Guard
ServerGoalHandle.publishFeedback()to only publish feedback while the goal isEXECUTING(otherwise warn and return). - Add tests covering feedback suppression outside executing state and duplicate
execute()calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/action/server_goal_handle.js | Adds state guards to prevent invalid execute/feedback operations that can crash rcl. |
| test/test-action-server.js | Adds regression tests for feedback gating and duplicate execute() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -87,6 +87,20 @@ class ServerGoalHandle { | |||
| * @returns {undefined} | |||
| */ | |||
| execute(callback) { | |||
There was a problem hiding this comment.
The execute() JSDoc says it “begins execution”, but the implementation now silently returns (and won’t invoke the callback) when the goal is already executing, inactive, or destroyed. Please update the method documentation to reflect these no-op cases so API consumers understand when execute() has an effect.
| this._actionServer._node | ||
| .getLogger() | ||
| .warn( | ||
| 'publishFeedback() called on a goal handle not in executing state, ignoring' |
There was a problem hiding this comment.
This warning doesn’t include any context (e.g., goal ID and current status), which makes it hard to diagnose which goal is misbehaving when multiple goals are active. Consider including goalId (and/or status) in the log message.
| 'publishFeedback() called on a goal handle not in executing state, ignoring' | |
| `publishFeedback() called on a goal handle not in executing state (goalId=${JSON.stringify( | |
| this.goalId | |
| )}, status=${this.status}), ignoring` |
| await assertUtils.createDelay(200); | ||
| assert.strictEqual(feedbackReceived, false); |
There was a problem hiding this comment.
This test adds a 200ms delay to check for absence of feedback; similar feedback tests in this file use shorter delays (e.g., 50ms). Consider reducing this delay (or reusing a shared constant) to keep the suite fast while still being reliable.
| await assertUtils.createDelay(200); | ||
| assert.ok(feedbackMessage); | ||
| assert.ok(deepEqual(Int32Array.from(sequence), feedbackMessage.sequence)); |
There was a problem hiding this comment.
This test adds a 200ms delay before asserting feedback was received; earlier feedback tests in this file use ~50ms. Consider lowering the delay (or using a shared helper/constant) to reduce overall test runtime.
| await assertUtils.createDelay(200); | ||
| assert.strictEqual(feedbackCount, 0); |
There was a problem hiding this comment.
This test uses a 200ms delay to confirm feedback isn’t published after a terminal state; consider reducing it (or using a shared constant) for faster, more consistent test timing across the suite.
Guard `publishFeedback()` to only publish when the goal is in EXECUTING state; log a warning and return early otherwise. Guard `execute()` to return early if the goal is already executing or no longer active, preventing invalid state transitions that crash rcl. Ported from rclpy commit f76729cf ("publish_feedback should effect only on executing state", ros2/rclpy#1639).
**Modified: `lib/action/server_goal_handle.js`** — `publishFeedback()` now checks `this.status !== STATUS_EXECUTING` and warns + returns if not executing. `execute()` now returns early if already executing or if the goal handle is no longer active (terminal state).
**Modified: `test/test-action-server.js`** — Added 4 tests: publishFeedback on accepted goal is ignored, publishFeedback on executing goal works normally, publishFeedback after succeed is ignored, and duplicate execute() is a no-op.
Fix: #1465
Guard
publishFeedback()to only publish when the goal is in EXECUTING state; log a warning and return early otherwise. Guardexecute()to return early if the goal is already executing or no longer active, preventing invalid state transitions that crash rcl. Ported from rclpy commit f76729cf ("publish_feedback should effect only on executing state", ros2/rclpy#1639).Modified:
lib/action/server_goal_handle.js—publishFeedback()now checksthis.status !== STATUS_EXECUTINGand warns + returns if not executing.execute()now returns early if already executing or if the goal handle is no longer active (terminal state).Modified:
test/test-action-server.js— Added 4 tests: publishFeedback on accepted goal is ignored, publishFeedback on executing goal works normally, publishFeedback after succeed is ignored, and duplicate execute() is a no-op.Fix: #1465