Defensively look up result future in _execute_get_result_request

`_execute_get_result_request` checks `_goal_handles` for the requested
goal ID before accessing `_result_futures`, but the two dicts are
managed separately and `_execute_expire_goals` removes entries from
both in another coroutine. When expiration runs between the existing
check and the access, the executor crashes with `KeyError`. Look up
the result future defensively and log a warning when it has already
been removed, matching the existing handling for unknown goal IDs
above.

Signed-off-by: Saad Sharif Ahmed <saad.sharifahmed@gmail.com>

Author: zeon01

rclpy/rclpy/action/server.py

@@ -548,9 +548,15 @@ async def _execute_get_result_request(
             return
 
         # There is an accepted goal matching the goal ID, register a callback to send the
-        # response as soon as it's ready
-        self._result_futures[bytes(goal_uuid)].add_done_callback(
-            functools.partial(self._send_result_response, request_header))
+        # response as soon as it's ready. The result future may have been removed by
+        # `_execute_expire_goals` between the membership check above and this access,
+        # so look it up defensively to avoid raising KeyError out of the executor.
+        if bytes(goal_uuid) in self._result_futures:
+            self._result_futures[bytes(goal_uuid)].add_done_callback(
+                functools.partial(self._send_result_response, request_header))
+        else:
+            self._logger.warning(
+                'Requested result has been expired already (goal ID: {0})'.format(goal_uuid))
 
     async def _execute_expire_goals(self, expired_goals: Tuple[GoalInfo, ...]) -> None:
         for goal in expired_goals:

rclpy/test/test_action_server.py

@@ -706,6 +706,58 @@ def test_expire_goals_multi(self) -> None:
         self.assertEqual(0, len(action_server._goal_handles))
         action_server.destroy()
 
+    def test_get_result_request_handles_expired_future(self) -> None:
+        """Regression test for ros2/rclpy#1236.
+
+        If ``_execute_expire_goals`` removes the result future from
+        ``_result_futures`` between the membership check on
+        ``_goal_handles`` and the access on ``_result_futures`` inside
+        ``_execute_get_result_request``, the action server must log a
+        warning and return rather than raising ``KeyError`` out of the
+        executor. The race is simulated deterministically here by
+        manipulating ``_result_futures`` directly.
+        """
+        action_server = ActionServer(
+            self.node,
+            Fibonacci,
+            'fibonacci',
+            execute_callback=self.execute_goal_callback,
+        )
+
+        goal_uuid = UUID(uuid=list(uuid.uuid4().bytes))
+        goal_msg = Fibonacci.Impl.SendGoalService.Request()
+        goal_msg.goal_id = goal_uuid
+        goal_future = self.mock_action_client.send_goal(goal_msg)
+        rclpy.spin_until_future_complete(self.node, goal_future, self.executor)
+        goal_handle = goal_future.result()
+        assert goal_handle
+        self.assertTrue(goal_handle.accepted)
+
+        # Let the executor run ``_execute_goal`` so the goal's result
+        # future is created and resolved before we tamper with state.
+        self.timed_spin(0.5)
+
+        uuid_bytes = bytes(goal_uuid.uuid)
+        self.assertIn(uuid_bytes, action_server._goal_handles)
+        self.assertIn(uuid_bytes, action_server._result_futures)
+
+        # Simulate the race: ``_execute_expire_goals`` removed the result
+        # future between the ``_goal_handles`` check and the access at
+        # the bottom of ``_execute_get_result_request``.
+        del action_server._result_futures[uuid_bytes]
+
+        # Without the defensive lookup this raises ``KeyError`` out of
+        # ``spin_once`` and crashes the executor.
+        get_result_future = self.mock_action_client.get_result(goal_uuid)
+        rclpy.spin_until_future_complete(
+            self.node, get_result_future, self.executor, timeout_sec=2)
+
+        # The defensive path logs a warning instead of sending a result
+        # response, so the client-side future never completes — the
+        # important assertion is that no exception was raised.
+        self.assertFalse(get_result_future.done())
+        action_server.destroy()
+
     def test_feedback(self) -> None:
 
         def execute_with_feedback(goal_handle: ServerGoalHandle[Fibonacci.Goal, Fibonacci.Result,