fix(gateway): make executor_threads real and report a cancel timeout as a timeout

.github/workflows/quality.yml

@@ -368,6 +368,14 @@ jobs:
           # new_delete_type_mismatch=0: ROS 2 DDS scalar/array new/delete mismatch
           ASAN_OPTIONS: halt_on_error=1:detect_leaks=0:new_delete_type_mismatch=0
           UBSAN_OPTIONS: halt_on_error=1:print_stacktrace=1
+          # Same factor as the ctest TIMEOUT rewrite above, for the wall-clock
+          # budgets tests assert internally - ctest's clock cannot reach those.
+          MEDKIT_TEST_TIME_SCALE: 3
+          # Lets tests size instrumented-only-expensive resources down (e.g. a
+          # 256-thread executor whose teardown outlives launch_testing's grace
+          # period under instrumentation). Detection also falls back to the
+          # sanitizer's own *SAN_OPTIONS, so this is belt and braces.
+          MEDKIT_TEST_SANITIZED: 1
         run: |
           source /opt/ros/jazzy/setup.bash
           source install/setup.bash
@@ -476,6 +484,15 @@ jobs:
 
       - name: Run unit + integration tests with TSan
         timeout-minutes: 30
+        env:
+          # Same factor as the ctest TIMEOUT rewrite above, for the wall-clock
+          # budgets tests assert internally - ctest's clock cannot reach those.
+          MEDKIT_TEST_TIME_SCALE: 3
+          # Lets tests size instrumented-only-expensive resources down (e.g. a
+          # 256-thread executor whose teardown outlives launch_testing's grace
+          # period under instrumentation). Detection also falls back to the
+          # sanitizer's own *SAN_OPTIONS, so this is belt and braces.
+          MEDKIT_TEST_SANITIZED: 1
         run: |
           export TSAN_OPTIONS="halt_on_error=0:history_size=4:suppressions=$(pwd)/tsan_suppressions.txt"
           source /opt/ros/jazzy/setup.bash

CONTRIBUTING.md

@@ -100,6 +100,28 @@ pre-commit install --hook-type pre-push
 On commit: clang-format, cmake-lint, shellcheck, flake8, ament-copyright, trailing whitespace.
 On push: incremental clang-tidy on changed `.cpp` files.
 
+#### Reproducing a sanitizer failure locally
+
+The ASan/TSan jobs multiply every declared CTest `TIMEOUT` by three, but a
+budget a test asserts on *itself* is invisible to that rewrite - an
+instrumented gateway can blow a "must answer within N seconds" assertion long
+before ctest's clock runs out, and the failure then reads as a product
+regression rather than as instrumentation overhead. Those budgets read
+`MEDKIT_TEST_TIME_SCALE`, which the sanitizer jobs export with the same factor.
+
+Set it when reproducing a sanitizer failure locally, or the run you get is not
+the run CI got:
+
+```bash
+MEDKIT_TEST_TIME_SCALE=3 colcon test --ctest-args -LE linter
+```
+
+It is honoured by both suites - Python integration tests via
+`ros2_medkit_test_utils.constants.get_time_scale()`, and C++ fixtures that wait
+on wall-clock budgets via their own local `test_time_scale()` helper. Unset,
+unparseable or below `1` means no scaling, so ordinary runs keep the tight
+budgets that give the assertions their falsifying power.
+
 #### Code Coverage
 
 Run from the workspace root. This mirrors the measurement pipeline of the CI

docs/api/rest.rst

@@ -688,20 +688,91 @@ Execute Operations
    .. code-block:: json
 
       {
-        "execution_id": "abc123-def456",
-        "status": "succeeded",
-        "result": {"sequence": [0, 1, 1, 2, 3, 5, 8, 13, 21, 34]},
-        "feedback": [
-          {"partial_sequence": [0, 1]},
-          {"partial_sequence": [0, 1, 1, 2, 3]}
-        ]
+        "status": "completed",
+        "capability": "execute",
+        "parameters": {"sequence": [0, 1, 1, 2, 3, 5, 8, 13, 21, 34]},
+        "x-medkit": {
+          "goal_id": "abc123def456789a0b1c2d3e4f506172",
+          "ros2_status": "succeeded",
+          "ros2": {
+            "action": "/powertrain/engine/long_calibration",
+            "type": "example_interfaces/action/Fibonacci"
+          }
+        }
       }
 
+   ``status`` carries the SOVD execution status and is one of ``pending``,
+   ``running``, ``completed``, ``failed``. ``parameters`` carries the action's
+   most recent feedback.
+
+   .. note::
+
+      **Reading the outcome of a cancel.** ``status`` cannot express it on its
+      own: a cancelled goal and a goal that failed by itself both render as
+      ``failed``, and a goal that is still cancelling renders as ``running``.
+      ``x-medkit.ros2_status`` carries the underlying ROS 2 goal state
+      verbatim - ``accepted``, ``executing``, ``canceling``, ``succeeded``,
+      ``canceled``, ``aborted`` - and is the field to read when a
+      ``DELETE``/``PUT``-stop answered ``504`` and the outcome has to be
+      established by polling.
+
+``PUT /api/v1/components/{id}/operations/{operation_id}/executions/{execution_id}``
+   Send a control command to a running execution. ROS 2 actions implement the
+   SOVD ``stop`` capability (mapped to action cancel):
+
+   .. code-block:: json
+
+      {"capability": "stop"}
+
+   - **202:** Stop accepted - the goal is cancelling; ``Location`` points at
+     the execution status resource. Also returned when the cancel response
+     was lost but the action's status stream already shows the goal
+     cancelling.
+   - **400:** The action server rejected the stop
+     (``x-medkit-ros2-action-rejected``, ``return_code`` 1-3), or the
+     capability is unsupported (``freeze`` / ``reset`` / unknown -
+     ``invalid-parameter``)
+   - **404:** Execution not found
+   - **409:** ``execute`` on an already-running execution
+     (``precondition-not-fulfilled``)
+   - **500:** Transport failure while sending the cancel
+     (``x-medkit-ros2-action-unavailable``)
+   - **503:** Cancel service not available - the action server is gone
+     (``x-medkit-ros2-action-unavailable``)
+   - **504:** No response from the action server within the cancel budget and
+     the status stream does not show the goal cancelling: the outcome is
+     unknown - poll the execution status resource (``not-responding``)
+
 ``DELETE /api/v1/components/{id}/operations/{operation_id}/executions/{execution_id}``
    Cancel a running execution.
 
-   - **204:** Execution cancelled
-   - **404:** Execution not found
+   - **204:** Execution cancelled. Also returned when the cancel response was
+     lost but the action's status stream already shows the goal cancelling.
+   - **400:** The action server answered and rejected the cancel
+     (``x-medkit-ros2-action-rejected``, ``return_code`` 1-3). Note
+     ``return_code`` 2 means the *action server* no longer knows the goal
+     while the gateway still tracks it - the request will not start
+     succeeding on retry.
+   - **404:** Execution not found - the *gateway* no longer tracks it
+     (``resource-not-found``)
+   - **500:** Transport failure while sending the cancel
+     (``x-medkit-ros2-action-unavailable``)
+   - **503:** Cancel service not available - the action server is gone
+     (``x-medkit-ros2-action-unavailable``)
+   - **504:** No response from the action server within the cancel budget and
+     the status stream does not show the goal cancelling: the outcome is
+     unknown - poll the execution status resource (``not-responding``)
+
+.. note::
+
+   **Cancel budget.** Both routes above are bounded by
+   ``service_call_timeout_sec`` (default 10 s, clamped to 1-3600; see
+   :doc:`../config/server`) plus up to 2 s spent discovering the action's
+   cancel service, so the worst case a client should allow is
+   ``service_call_timeout_sec + 2 s``. Configuring a budget shorter than that
+   discovery wait does not shorten the discovery wait - a cancel issued before
+   the cancel service has been discovered still spends up to 2 s there before
+   the response wait starts.
 
 Lifecycle Endpoints
 -------------------
@@ -2485,35 +2556,44 @@ All error responses follow a consistent format:
 Common Error Codes
 ~~~~~~~~~~~~~~~~~~
 
+Standard SOVD codes appear in the response's ``error_code`` field.
+Vendor-specific ``x-medkit-*`` codes are enveloped: the response carries
+``error_code: "vendor-error"`` with the precise code in ``vendor_code``.
+
 .. list-table::
    :header-rows: 1
    :widths: 30 15 55
 
    * - Error Code
      - HTTP Status
      - Description
-   * - ``ERR_ENTITY_NOT_FOUND``
+   * - ``entity-not-found``
      - 404
      - The requested entity does not exist
-   * - ``ERR_RESOURCE_NOT_FOUND``
+   * - ``resource-not-found``
      - 404
      - The requested resource (topic, service, parameter) does not exist
-   * - ``ERR_INVALID_INPUT``
+   * - ``invalid-request``
      - 400
-     - Invalid request body or parameters
-   * - ``ERR_INVALID_ENTITY_ID``
+     - Invalid request body or missing required parameters
+   * - ``precondition-not-fulfilled``
+     - 409
+     - The resource's current state does not allow the request - e.g.
+       ``execute`` on an execution that is still running
+   * - ``invalid-parameter``
      - 400
-     - Entity ID contains invalid characters
-   * - ``ERR_OPERATION_FAILED``
+     - Invalid parameter value (including malformed entity IDs)
+   * - ``internal-error``
      - 500
-     - Operation failed during execution
-   * - ``ERR_TIMEOUT``
+     - Internal server error
+   * - ``not-responding``
      - 504
-     - Operation timed out
-   * - ``ERR_UNAUTHORIZED``
+     - The underlying ROS 2 entity did not respond in time; the outcome of
+       the request is unknown
+   * - ``unauthorized``
      - 401
      - Authentication required or token invalid
-   * - ``ERR_FORBIDDEN``
+   * - ``forbidden``
      - 403
      - Insufficient permissions for this operation
    * - ``x-medkit-plugin-error``

docs/config/server.rst

@@ -166,6 +166,47 @@ Data Access Settings
      - After a node's parameter service fails to respond, subsequent requests
        return immediately with SERVICE_UNAVAILABLE for this duration.
        Set to 0 to disable. Range: 0-3600.
+   * - ``service_call_timeout_sec``
+     - int
+     - ``10``
+     - How long the gateway waits for a **response** to an operation RPC: a
+       ROS 2 service call (``POST .../executions`` on a service-backed
+       operation) and each of the three action RPCs - send goal, get result
+       and cancel. Values outside the range are clamped with a warning at
+       startup. Range: 1-3600.
+
+       It is not the whole wall-clock cost, because each RPC first waits for
+       its service to be discovered and the three do that differently:
+
+       .. list-table::
+          :header-rows: 1
+          :widths: 30 35 35
+
+          * - RPC
+            - Discovery wait
+            - Worst case in total
+          * - Service call
+            - none (bounded by the response wait)
+            - ``service_call_timeout_sec``
+          * - Action send goal
+            - up to ``service_call_timeout_sec``
+            - ``2 x service_call_timeout_sec``
+          * - Action get result
+            - up to 2 s, fixed
+            - ``service_call_timeout_sec + 2 s``
+          * - Action cancel
+            - up to 2 s, fixed
+            - ``service_call_timeout_sec + 2 s``
+
+       The last row is the *cancel budget* referenced by
+       ``DELETE .../executions/{id}`` and ``PUT .../executions/{id}`` (see
+       :doc:`../api/rest`): a cancel that gets no answer within the response
+       wait is reported as ``504 not-responding`` unless the action's status
+       stream already shows the goal cancelling. The 2 s discovery waits are
+       fixed and do not shrink with this parameter, so lowering it to the
+       minimum of 1 s does not make an undiscovered cancel or get-result
+       answer in under 2 s - size client timeouts off the "worst case in
+       total" column, not off the parameter alone.
 
 .. note::
 
@@ -338,7 +379,10 @@ read, so a mis-set parameter can never break request serving.
      - int
      - ``2``
      - Threads in the main rclcpp ``MultiThreadedExecutor``. Replaces rclcpp's
-       default (host cores, minimum 2). Clamped to ``[1, 256]``.
+       default (host cores, minimum 2). With two or more threads,
+       blocking-RPC responses (operation executions) are dispatched even
+       while other gateway callbacks run (see note below). Clamped to
+       ``[1, 256]``.
 
 **HTTP pool, keep-alive, and SSE.** Several things hold an HTTP pool worker:
 each active SSE stream (fault dashboard, cyclic subscriptions, trigger events -
@@ -361,14 +405,24 @@ connection reuse).
 
 **Executor threads.** The main executor delivers the gateway node's own
 callbacks (timers, graph events, log and fault subscriptions) and the
-service-response callbacks that complete operation/action RPC futures. These all
-run on the node's default, *mutually-exclusive* callback group, so they serialize
-through a single thread regardless of ``executor_threads`` - raising it buys no
-RPC-response parallelism. A small executor is safe because the blocking wait for
+service-response callbacks that complete operation/action RPC futures. The RPC
+response callbacks - the generic service clients behind ``/operations``
+executions and the per-action send_goal / get_result / cancel_goal client trio -
+run in a shared *reentrant* callback group, so with two or more executor threads
+a response is dispatched even while another gateway callback (for example a
+discovery refresh pass) is running: the default of ``2`` buys real RPC-response
+parallelism. Timers and the SSE-fault / trigger-fault / ``/rosout``
+subscriptions stay in the node's default, *mutually-exclusive* group by design -
+discovery refresh passes are serialized on purpose, and those subscriptions rely
+on in-order delivery. Per-action ``/_action/status`` subscriptions use a
+dedicated mutually-exclusive group of their own: ordered among themselves,
+decoupled from the default group. A single executor thread remains safe (a
+reentrant group does not *require* a second thread), and the blocking wait for
 an RPC runs on the cpp-httplib pool thread (a separate server thread), never on
 an executor thread, so it cannot deadlock the executor; the fault transport also
-uses its own private executor. Increase this only if the node's own callback load
-grows (for example very frequent graph churn).
+uses its own private executor. Raise this beyond ``2`` if many concurrent RPC
+responses must be dispatched in parallel or the node's own callback load grows
+(for example very frequent graph churn).
 
 Example (more SSE clients needs a larger pool and matching ``sse.max_clients``):
 

src/ros2_medkit_cmake/cmake/ROS2MedkitLinting.cmake

@@ -19,11 +19,24 @@ include_guard(GLOBAL)
 # (alongside include(ROS2MedkitCcache) - CMAKE_MODULE_PATH is already set).
 #
 # Provides:
+#   CMAKE_EXPORT_COMPILE_COMMANDS ON
 #   function medkit_lint_config(<file name> <output variable>)
 #   option ENABLE_CLANG_TIDY (default OFF)
 #   cache var ROS2_MEDKIT_CLANG_TIDY_JOBS (default: host core count)
 #   function ros2_medkit_clang_tidy([HEADER_FILTER <regex>] [TIMEOUT <seconds>] [JOBS <n>])
 
+# Every package that lints needs a compile database, because that is where
+# clang-tidy reads a file's include paths and flags from. Setting it per package
+# meant a new package started life invisible to the analysis: scripts/
+# clang-tidy-diff.sh merges the per-package databases, and a file absent from
+# the merged database is analysed with no flags at all. It then fails to find
+# its own headers, and the pre-push hook reports that parse error instead of
+# the checks it was meant to run. Eight of the packages set this and the rest
+# did not, so most of the workspace was never really analysed. Setting it here
+# means including this module is what makes a package lintable, with nothing
+# else to remember.
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+
 # The shared lint configurations - .clang-format, .clang-tidy, .flake8 - are
 # files of this package, kept next to the cmake modules and installed next to
 # them, so the same relative lookup works from a source tree, from a plain

src/ros2_medkit_gateway/CMakeLists.txt

@@ -197,6 +197,7 @@ add_library(gateway_ros2 STATIC
   src/plugins/plugin_http_types.cpp
   src/plugins/plugin_loader.cpp
   src/plugins/plugin_manager.cpp
+  src/ros2_common/callback_groups.cpp
   src/ros2_common/ros2_subscription_executor.cpp
   src/ros2_common/ros2_subscription_slot.cpp
   src/script_manager.cpp
@@ -842,6 +843,19 @@ if(BUILD_TESTING)
   target_link_libraries(test_operation_handlers gateway_ros2)
   medkit_target_dependencies(test_operation_handlers rclcpp rclcpp_action std_srvs example_interfaces)
 
+  # Cancel-outcome falsifiers (issue #576). The fixture's CancelGoal service
+  # deliberately parks requests past the configured budget, so the suite
+  # spends multiple seconds inside blocking waits by design - give it
+  # headroom over the default gtest timeout.
+  medkit_add_gtest(test_cancel_outcomes test/test_cancel_outcomes.cpp TIMEOUT 180)
+  target_link_libraries(test_cancel_outcomes gateway_ros2)
+  medkit_target_dependencies(test_cancel_outcomes rclcpp action_msgs)
+
+  # Callback-group wiring contract (issue #575)
+  medkit_add_gtest(test_callback_groups test/test_callback_groups.cpp)
+  target_link_libraries(test_callback_groups gateway_ros2)
+  medkit_target_dependencies(test_callback_groups rclcpp action_msgs)
+
   # Demo update backend plugin (.so for integration tests)
   add_library(test_update_backend MODULE
     test/demo_nodes/test_update_backend.cpp