Fix action server goal expiration and correct broken test assertions (#1420)

## fix: action server goal expiration never removed expired goals

### Summary

Fixed two bugs in the action server goal expiration mechanism that caused expired goals to never be removed from memory, and a crash when they were. Also fixed 9 tautological test assertions that masked both bugs, and added deserialized goal info validation to the multi-goal expiration test.

### Bug 1 — Wrong buffer read in `_executeExpiredGoals` (`lib/action/server.js`)

`_executeExpiredGoals()` deserialized goal info from `result.data[i].refObject`, which are JavaScript wrapper objects each with their own empty `_refObject` buffer. The native `rcl_action_expire_goals()` writes into `result._refArray.buffer`, a completely separate memory block. The wrappers never see the native data, so the deserialized UUID was always all-zeros and `_goalHandles.delete(uuid)` never matched — goals were silently never expired.

**Fix:** Changed `goal.refObject` → `result._refArray[i]` to read directly from the native-written buffer.

### Bug 2 — Crash on zero-capacity buffer (`lib/node.js`)

After Bug 1 is fixed, expired goals are actually removed from `_goalHandles`. On the next spin cycle, `isGoalExpired` may still fire with `_goalHandles.size == 0`. Passing a zero-capacity buffer to `rcl_action_expire_goals` violates an RCL precondition and crashes:
> `expired_goals, expired_goals_capacity, and num_expired inconsistent`

**Fix:** Added `if (numGoals > 0)` guard to skip the native call when there are no goal handles.

### Bug 3 — Tautological test assertions (`test/test-action-server.js`)

9 instances of `assert.ok(value, expected)` where `assert.ok`'s second argument is a failure message, not a comparison. These always passed regardless of actual values.

**Fix:** Changed all 9 to `assert.strictEqual(value, expected)`.

### Enhanced test validation (`test/test-action-server.js`)

Added goal info validation to "Test expire goals multi" that wraps `_executeExpiredGoals` to intercept and verify the deserialized data from the native buffer:

- **Count:** exactly 3 expired goals reported
- **Non-zero UUIDs:** each deserialized UUID is not all-zeros (catches the original bug)
- **Uniqueness:** all 3 UUIDs are distinct (no buffer aliasing)
- **UUID match:** expired UUIDs are byte-identical to the accepted goal UUIDs
- **Non-zero stamps:** each `GoalInfo.stamp` has a non-zero acceptance time (validates full struct deserialization)

Also increased expiration delays from 1000ms to 3000ms for `resultTimeout: 1` (1 second) to provide adequate margin.

### Files changed

| File | Changes |
|------|---------|
| `lib/action/server.js` | Read from `result._refArray[i]` instead of `result.data[i].refObject` |
| `lib/node.js` | Guard `actionExpireGoals` call with `if (numGoals > 0)` |
| `test/test-action-server.js` | Fix 9 `assert.ok` → `assert.strictEqual`; add deserialized goal info validation; increase expire delays |

Fix: #1419

Author: minggangw

lib/action/server.js

@@ -437,10 +437,8 @@ class ActionServer extends Entity {
 
   _executeExpiredGoals(result, count) {
     for (let i = 0; i < count; i++) {
-      const goal = result.data[i];
-
       const goalInfo = new ActionInterfaces.GoalInfo();
-      goalInfo.deserialize(goal.refObject);
+      goalInfo.deserialize(result._refArray[i]);
 
       let uuid = ActionUuid.fromBytes(goalInfo.goal_id.uuid).toString();
       this._goalHandles.delete(uuid);

lib/node.js

@@ -425,17 +425,20 @@ class Node extends rclnodejs.ShadowNode {
       }
 
       if (properties.isGoalExpired) {
-        let GoalInfoArray = ActionInterfaces.GoalInfo.ArrayType;
-        let message = new GoalInfoArray(actionServer._goalHandles.size);
-        let count = rclnodejs.actionExpireGoals(
-          actionServer.handle,
-          actionServer._goalHandles.size,
-          message._refArray.buffer
-        );
-        if (count > 0) {
-          actionServer.processGoalExpired(message, count);
+        let numGoals = actionServer._goalHandles.size;
+        if (numGoals > 0) {
+          let GoalInfoArray = ActionInterfaces.GoalInfo.ArrayType;
+          let message = new GoalInfoArray(numGoals);
+          let count = rclnodejs.actionExpireGoals(
+            actionServer.handle,
+            numGoals,
+            message._refArray.buffer
+          );
+          if (count > 0) {
+            actionServer.processGoalExpired(message, count);
+          }
+          GoalInfoArray.freeArray(message);
         }
-        GoalInfoArray.freeArray(message);
       }
     }
 

test/test-action-server.js

@@ -446,7 +446,7 @@ describe('rclnodejs action server', function () {
 
     let result = await handle.getResult();
     assert.ok(result);
-    assert.ok(handle.status, GoalStatus.STATUS_SUCCEEDED);
+    assert.strictEqual(handle.status, GoalStatus.STATUS_SUCCEEDED);
     assert.ok(deepEqual(result.sequence, Int32Array.from(testSequence)));
 
     server.destroy();
@@ -480,7 +480,7 @@ describe('rclnodejs action server', function () {
 
     let result = await handle.getResult();
     assert.ok(result);
-    assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
+    assert.strictEqual(handle.status, GoalStatus.STATUS_ABORTED);
     assert.ok(deepEqual(result.sequence, Int32Array.from(testSequence)));
 
     server.destroy();
@@ -515,7 +515,7 @@ describe('rclnodejs action server', function () {
     let result = await handle.getResult();
     assert.ok(result);
     // Goal status should default to aborted
-    assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
+    assert.strictEqual(handle.status, GoalStatus.STATUS_ABORTED);
     assert.ok(deepEqual(result.sequence, Int32Array.from(testSequence)));
 
     server.destroy();
@@ -543,7 +543,7 @@ describe('rclnodejs action server', function () {
     let result = await handle.getResult();
     assert.ok(result);
     // Goal status should default to aborted
-    assert.ok(handle.status, GoalStatus.STATUS_ABORTED);
+    assert.strictEqual(handle.status, GoalStatus.STATUS_ABORTED);
     assert.ok(deepEqual(result.sequence, Int32Array.from([])));
 
     server.destroy();
@@ -567,7 +567,7 @@ describe('rclnodejs action server', function () {
     await client.sendGoal(goal);
 
     await assertUtils.createDelay(500);
-    assert.ok(server._goalHandles.size, 1);
+    assert.strictEqual(server._goalHandles.size, 1);
 
     server.destroy();
   });
@@ -590,10 +590,10 @@ describe('rclnodejs action server', function () {
     await client.sendGoal(goal);
 
     await assertUtils.createDelay(500);
-    assert.ok(server._goalHandles.size, 1);
+    assert.strictEqual(server._goalHandles.size, 1);
 
-    await assertUtils.createDelay(1000);
-    assert.ok(server._goalHandles.size, 0);
+    await assertUtils.createDelay(3000);
+    assert.strictEqual(server._goalHandles.size, 0);
 
     server.destroy();
   });
@@ -610,6 +610,24 @@ describe('rclnodejs action server', function () {
       { resultTimeout: 1 }
     );
 
+    // Wrap _executeExpiredGoals to capture the goalInfo deserialized from the
+    // native buffer, so we can verify they match the accepted goals.
+    const expiredGoalInfos = [];
+    const origExecuteExpiredGoals = server._executeExpiredGoals.bind(server);
+    server._executeExpiredGoals = function (result, count) {
+      const ActionInterfaces = require('../lib/action/interfaces.js');
+      const ActionUuid = require('../lib/action/uuid.js');
+      for (let i = 0; i < count; i++) {
+        const goalInfo = new ActionInterfaces.GoalInfo();
+        goalInfo.deserialize(result._refArray[i]);
+        expiredGoalInfos.push({
+          uuid: ActionUuid.fromBytes(goalInfo.goal_id.uuid).toString(),
+          stamp: goalInfo.stamp,
+        });
+      }
+      origExecuteExpiredGoals(result, count);
+    };
+
     let goal = new Fibonacci.Goal();
 
     await client.waitForServer(1000);
@@ -620,10 +638,37 @@ describe('rclnodejs action server', function () {
     ]);
 
     await assertUtils.createDelay(500);
-    assert.ok(server._goalHandles.size, 3);
+    assert.strictEqual(server._goalHandles.size, 3);
+
+    // Snapshot the accepted goal UUIDs before expiration
+    const acceptedUuids = Array.from(server._goalHandles.keys()).sort();
+
+    await assertUtils.createDelay(3000);
+    assert.strictEqual(server._goalHandles.size, 0);
+
+    // Verify the expired goal info deserialized from the native buffer
+    assert.strictEqual(expiredGoalInfos.length, 3);
+
+    const expiredUuids = expiredGoalInfos.map((g) => g.uuid).sort();
 
-    await assertUtils.createDelay(1000);
-    assert.ok(server._goalHandles.size, 0);
+    // Each UUID must be non-zero (the original bug produced all-zero UUIDs)
+    for (const info of expiredGoalInfos) {
+      assert.notStrictEqual(info.uuid, Array(16).fill(0).join(','));
+    }
+
+    // All 3 UUIDs must be unique (3 distinct goals)
+    assert.strictEqual(new Set(expiredUuids).size, 3);
+
+    // The expired UUIDs must match the originally accepted ones
+    assert.deepStrictEqual(expiredUuids, acceptedUuids);
+
+    // Each expired goal must have a non-zero acceptance stamp
+    for (const info of expiredGoalInfos) {
+      assert.ok(
+        info.stamp.sec > 0 || info.stamp.nanosec > 0,
+        `Expected non-zero stamp, got sec=${info.stamp.sec} nanosec=${info.stamp.nanosec}`
+      );
+    }
 
     server.destroy();
   });