Lock the goal-handle / result-future registry in ActionServer

[zeon01]

Summary

Closes #1236. Closes #1667.

ActionServer tracks goals in two dicts — _goal_handles and _result_futures — that are mutated and read as a pair. With the MultiThreadedExecutor, _execute_expire_goals can remove a goal from both dicts on one thread while _execute_goal, _execute_get_result_request, _execute_cancel_request or ServerGoalHandle._set_result reads them on another. The reader then hits a KeyError and the executor crashes.

This adds _goal_registry_lock and takes it around every access to the dict pair, so a reader never observes one dict updated without the other.

Design notes

• threading.Lock, not asyncio.Lock. MultiThreadedExecutor steps these coroutines on real OS threads; asyncio.Lock is not thread-safe. The race holds even under MutuallyExclusiveCallbackGroup, because _execute_goal is scheduled as its own executor task via notify_execute (executor.create_task) and is therefore not serialized against the waitable's _execute_expire_goals by the callback group.
• The lock is only ever held across synchronous sections — no await, no user-callback invocation. Where a coroutine acts on a Future, the reference is fetched under the lock and set_result() / add_done_callback() is called after release.
• It is a separate lock from the existing self._lock and the two are never nested, so it cannot deadlock.
• With the registry consistent, _execute_get_result_request reliably detects an expired goal and returns STATUS_UNKNOWN instead of crashing.

This replaces the earlier defensive-lookup-only revision of this PR, per review feedback that the root-cause fix is locking the dict pair as a single unit of state.

Test plan

Two regression tests added to test_action_server.py:

• test_get_result_request_handles_expired_future (KeyError in ActionServer._execute_goal #1236) — removes the result future, sends a get-result request; asserts a STATUS_UNKNOWN response instead of a crash.
• test_execute_goal_handles_expired_future (KeyError in ActionServer._execute_goal under goal-timeout abort (race on _result_futures) #1667) — removes the result future while the execute callback runs; asserts _execute_goal does not raise.

Verified against osrf/ros:rolling-desktop:

• Without the patch: both new tests fail with KeyError (_execute_goal, _execute_get_result_request).
• With the patch: both pass.
• Full test_action_server.py — 28 tests — pass. ament_flake8 / ament_pep257 / ament_copyright pass.

Out of scope / follow-ups

• A cleaner long-term shape is to collapse the two dicts into a single Dict[bytes, GoalEntry]; kept out of this PR to stay minimal — can follow up if preferred.
• succeed(response) / abort(response) set the result future and _execute_goal then sets it again; rclpy's Future.set_result() overwrites + re-fires callbacks rather than raising, so the result can be sent twice. Separate issue from this race.

[fujitatomoya]

@zeon01 thanks for the patch.

i believe this is a defensive patch, this could mitigate the situation a bit, but not a real fix.
to address and get to the root cause, i think that the proper fix is an asyncio.Lock (or threading.Lock depending on which executor path can touch these) that covers the pair of dicts as a single unit of state.

[fujitatomoya]

CC: @Barry-Xu-2018

[zeon01]

Reworked to a lock-based fix — _goal_registry_lock now covers the _goal_handles / _result_futures pair as a single unit of state.

It's a threading.Lock: MultiThreadedExecutor steps these coroutines on real OS threads, and asyncio.Lock is not thread-safe. The race holds even under MutuallyExclusiveCallbackGroup — _execute_goal is scheduled as its own executor task via notify_execute (executor.create_task), so the callback group does not serialize it against the waitable's _execute_expire_goals.

The lock is taken around every access to the pair (_execute_goal_request, _execute_goal, _execute_get_result_request, _execute_cancel_request, _execute_expire_goals, ServerGoalHandle._set_result, destroy()). It is only ever held across synchronous sections — no await, no user-callback invocation: a Future reference is fetched under the lock and set_result() / add_done_callback() called after release, so done-callbacks can't re-enter it. It is disjoint from self._lock and never nested.

Verified: regression tests for #1236 and #1667 fail with KeyError against the current code and pass with the lock; the full test_action_server.py suite (28 tests) and the flake8/pep257/copyright linters pass.

Two out-of-scope notes:

• A cleaner long-term shape would collapse _goal_handles and _result_futures into a single Dict[bytes, GoalEntry] so the pair can't desync structurally. I kept this PR to the lock to stay minimal — happy to follow up if you'd prefer.
• ServerGoalHandle.succeed(response) / abort(response) call _set_result, and _execute_goal then sets the same future again, so passing a non-None response sets the future twice. Future.set_result() doesn't raise on a second call — it overwrites the result and re-fires done callbacks — so the result response can be sent twice. Separate from this race; happy to file an issue if it isn't already tracked.