Guard publishFeedback and execute for valid goal states (#1466)

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

Author: minggangw

.github/workflows/linux-x64-build-and-test.yml

@@ -80,7 +80,7 @@ jobs:
         # Install any remaining runtime dependencies using rosdep
         rosdep init || true
         rosdep update
-        rosdep install --rosdistro rolling --from-paths /opt/ros/rolling/share --ignore-src -y --skip-keys "cyclonedds fastcdr fastdds iceoryx_binding_c rmw_connextdds rti-connext-dds-7.3.0 urdfdom_headers"
+        rosdep install --rosdistro rolling --from-paths /opt/ros/rolling/share --ignore-src -y --skip-keys "cyclonedds fastcdr fastdds iceoryx_binding_c rmw_connextdds rti-connext-dds-7.3.0 urdfdom_headers python3-pyqt6.qtsvg"
 
     - name: Install test-msgs and mrpt_msgs on Linux
       if: ${{ matrix.ros_distribution != 'rolling' }}

lib/action/server_goal_handle.js

@@ -82,11 +82,26 @@ class ServerGoalHandle {
   }
 
   /**
-   * Updates the goal handle with the execute status and begins exection.
+   * Transitions the goal to the executing state and begins execution.
+   * Has no effect if the goal is already executing, no longer active, or destroyed.
    * @param {function} callback - An optional callback to use instead of the one provided to the action server.
    * @returns {undefined}
    */
   execute(callback) {
+    if (this._destroyed) {
+      return;
+    }
+
+    // Guard: already executing — no-op
+    if (this.status === ActionInterfaces.GoalStatus.STATUS_EXECUTING) {
+      return;
+    }
+
+    // Guard: only transition if goal is still active
+    if (!this.isActive) {
+      return;
+    }
+
     if (!this.isCancelRequested) {
       this._updateState(GoalEvent.EXECUTE);
     }
@@ -105,6 +120,16 @@ class ServerGoalHandle {
       return;
     }
 
+    if (this.status !== ActionInterfaces.GoalStatus.STATUS_EXECUTING) {
+      this._actionServer._node
+        .getLogger()
+        .warn(
+          `publishFeedback() called on goal ${this.goalId.uuid} ` +
+            `in status ${this.status}, not executing; ignoring`
+        );
+      return;
+    }
+
     let feedbackMessage =
       new this._actionServer.typeClass.impl.FeedbackMessage();
     feedbackMessage['goal_id'] = this.goalId;

test/test-action-server.js

@@ -773,4 +773,160 @@ describe('rclnodejs action server', function () {
       ServiceIntrospectionStates.CONTENTS
     );
   });
+
+  it('publishFeedback on accepted (not executing) goal should be ignored', async function () {
+    let serverGoalHandle;
+
+    function handleAcceptedCallback(goalHandle) {
+      serverGoalHandle = goalHandle;
+    }
+
+    let server = new rclnodejs.ActionServer(
+      node,
+      fibonacci,
+      'fibonacci',
+      (goalHandle) => {
+        goalHandle.succeed();
+        return new Fibonacci.Result();
+      },
+      null,
+      handleAcceptedCallback
+    );
+
+    let goal = new Fibonacci.Goal();
+    await client.waitForServer(1000);
+
+    let feedbackReceived = false;
+    const handle = await client.sendGoal(goal, () => {
+      feedbackReceived = true;
+    });
+    assert.ok(handle.accepted);
+    assert.strictEqual(serverGoalHandle.status, GoalStatus.STATUS_ACCEPTED);
+
+    // publishFeedback on accepted goal should not publish
+    const feedback = new Fibonacci.Feedback();
+    feedback.sequence = [1, 1, 2, 3];
+    serverGoalHandle.publishFeedback(feedback);
+
+    await assertUtils.createDelay(50);
+    assert.strictEqual(feedbackReceived, false);
+
+    // Clean up: execute and succeed
+    serverGoalHandle.execute();
+    await handle.getResult();
+    server.destroy();
+  });
+
+  it('publishFeedback on executing goal should publish normally', async function () {
+    const sequence = [1, 1, 2, 3];
+
+    function executeFeedbackCallback(goalHandle) {
+      assert.strictEqual(goalHandle.status, GoalStatus.STATUS_EXECUTING);
+
+      const feedback = new Fibonacci.Feedback();
+      feedback.sequence = sequence;
+      goalHandle.publishFeedback(feedback);
+      goalHandle.succeed();
+
+      return new Fibonacci.Result();
+    }
+
+    let server = new rclnodejs.ActionServer(
+      node,
+      fibonacci,
+      'fibonacci',
+      executeFeedbackCallback
+    );
+
+    let goal = new Fibonacci.Goal();
+    await client.waitForServer(1000);
+
+    let feedbackMessage;
+    const handle = await client.sendGoal(
+      goal,
+      (feedback) => (feedbackMessage = feedback)
+    );
+    assert.ok(handle.accepted);
+
+    await assertUtils.createDelay(50);
+    assert.ok(feedbackMessage);
+    assert.ok(deepEqual(Int32Array.from(sequence), feedbackMessage.sequence));
+
+    server.destroy();
+  });
+
+  it('publishFeedback after succeed should be ignored', async function () {
+    let serverGoalHandle;
+
+    function executeCallback(goalHandle) {
+      serverGoalHandle = goalHandle;
+      goalHandle.succeed();
+      return new Fibonacci.Result();
+    }
+
+    let server = new rclnodejs.ActionServer(
+      node,
+      fibonacci,
+      'fibonacci',
+      executeCallback
+    );
+
+    let goal = new Fibonacci.Goal();
+    await client.waitForServer(1000);
+
+    let feedbackCount = 0;
+    const handle = await client.sendGoal(goal, () => {
+      feedbackCount++;
+    });
+    assert.ok(handle.accepted);
+
+    await handle.getResult();
+    assert.strictEqual(serverGoalHandle.status, GoalStatus.STATUS_SUCCEEDED);
+
+    // publishFeedback after terminal state should not publish
+    const feedback = new Fibonacci.Feedback();
+    feedback.sequence = [1, 1, 2, 3];
+    serverGoalHandle.publishFeedback(feedback);
+
+    await assertUtils.createDelay(50);
+    assert.strictEqual(feedbackCount, 0);
+
+    server.destroy();
+  });
+
+  it('execute() on already executing goal should be a no-op', async function () {
+    let executeCalled = 0;
+
+    function handleAcceptedCallback(goalHandle) {
+      goalHandle.execute();
+      // Call execute again — should be a no-op
+      goalHandle.execute();
+    }
+
+    function executeCallback(goalHandle) {
+      executeCalled++;
+      goalHandle.succeed();
+      return new Fibonacci.Result();
+    }
+
+    let server = new rclnodejs.ActionServer(
+      node,
+      fibonacci,
+      'fibonacci',
+      executeCallback,
+      null,
+      handleAcceptedCallback
+    );
+
+    let goal = new Fibonacci.Goal();
+    await client.waitForServer(1000);
+
+    const handle = await client.sendGoal(goal);
+    assert.ok(handle.accepted);
+
+    await handle.getResult();
+    assert.strictEqual(executeCalled, 1);
+
+    server.destroy();
+  });
 });