diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index a5f0a3b23..1c85018a5 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -342,6 +342,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 @@ -445,6 +453,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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c4ea23274..339387dc6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,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 diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 663a0875c..6924a69fd 100644 --- a/docs/api/rest.rst +++ b/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,6 +2556,10 @@ 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 @@ -2492,28 +2567,33 @@ Common Error Codes * - 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`` diff --git a/docs/config/server.rst b/docs/config/server.rst index 027442a42..eb5b864f9 100644 --- a/docs/config/server.rst +++ b/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``): diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index 0d887ad71..31f686a40 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/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 @@ -855,6 +856,21 @@ if(BUILD_TESTING) medkit_target_dependencies(test_operation_handlers rclcpp rclcpp_action std_srvs example_interfaces) medkit_set_test_domain(test_operation_handlers) + # 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. + ament_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) + medkit_set_test_domain(test_cancel_outcomes) + + # Callback-group wiring contract (issue #575) + ament_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) + medkit_set_test_domain(test_callback_groups) + # Demo update backend plugin (.so for integration tests) add_library(test_update_backend MODULE test/demo_nodes/test_update_backend.cpp diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index 7a85a778b..39933e1ac 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -509,13 +509,36 @@ Cancel a running action execution. curl -X DELETE http://localhost:8080/api/v1/components/long_calibration/operations/long_calibration/executions/abc123def456 ``` -**Response (200 OK):** -```json -{ - "status": "canceling", - "goal_id": "abc123def456...", - "message": "Cancel request sent" -} +**Response:** `204 No Content` (empty body) - the cancellation is underway. Also +returned when the cancel response was lost but the action's status stream +already shows the goal cancelling. + +**Other outcomes:** + +| Status | Meaning | +|--------|---------| +| `400` | The action server answered and refused (`x-medkit-ros2-action-rejected`, `return_code` 1-3) | +| `404` | No such execution (`resource-not-found`) - including one evicted while the request was in flight | +| `500` | The cancel could not be delivered or parsed (`x-medkit-ros2-action-unavailable`) | +| `503` | The cancel service is gone - the action server died (`x-medkit-ros2-action-unavailable`) | +| `504` | No answer within the cancel budget and the status stream does not show the goal cancelling, so the outcome is unknown (`not-responding`) | + +The cancel budget is `service_call_timeout_sec` (default 10 s) plus up to 2 s of +cancel-service discovery. On `504`, poll the execution resource above and read +`x-medkit.ros2_status` - the SOVD `status` field renders both `CANCELED` and +`ABORTED` as `failed`, so it cannot tell you whether the cancellation took +effect. + +#### PUT /api/v1/components/{component_id}/operations/{operation_id}/executions/{execution_id} + +Send a control command to a running execution. ROS 2 actions implement the SOVD +`stop` capability, which maps to action cancel and shares the outcome table +above (with `202 Accepted` in place of `204`, plus `409` when `execute` is +requested on an execution that is still running). + +```bash +curl -X PUT -H 'Content-Type: application/json' -d '{"capability": "stop"}' \ + http://localhost:8080/api/v1/components/long_calibration/operations/long_calibration/executions/abc123def456 ``` ### Authentication Endpoints diff --git a/src/ros2_medkit_gateway/config/gateway_params.yaml b/src/ros2_medkit_gateway/config/gateway_params.yaml index 4b0c29609..6aa443f0e 100644 --- a/src/ros2_medkit_gateway/config/gateway_params.yaml +++ b/src/ros2_medkit_gateway/config/gateway_params.yaml @@ -50,11 +50,14 @@ ros2_medkit_gateway: keep_alive_timeout_sec: 2 # Number of threads in the main rclcpp MultiThreadedExecutor. This - # executor only dispatches the gateway node's own callbacks (timers, - # graph events, log/fault subscriptions); HTTP handlers that issue ROS - # service calls use their own private executors, so a small pool here is - # safe. Bounded to a small fixed size instead of std::thread:: - # hardware_concurrency(). Clamped to [1, 256]. + # executor dispatches the gateway node's own callbacks (timers, graph + # events, log/fault subscriptions) plus the response callbacks that + # complete operation/action RPC futures. The RPC response callbacks run + # in a shared reentrant callback group, so with 2+ threads a service or + # action response is delivered even while another gateway callback + # (e.g. a discovery refresh pass) is running. Bounded to a small fixed + # size instead of std::thread::hardware_concurrency(). + # Clamped to [1, 256]. # Default: 2 executor_threads: 2 diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/compat/generic_client_compat.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/compat/generic_client_compat.hpp index a7a704ef4..b5ca479ba 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/compat/generic_client_compat.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/compat/generic_client_compat.hpp @@ -26,6 +26,12 @@ /// When HAS_GENERIC_CLIENT is false (Humble), GenericServiceClient is a custom class /// that replicates GenericClient's behavior using rcl C APIs and the same /// rosidl_typesupport_introspection infrastructure available in all distros. +/// +/// Both paths take the callback group the client's response callback should +/// be dispatched on (issue #575: the gateway registers its blocking-RPC +/// clients into a shared Reentrant group so responses are not serialized +/// behind the node's default MutuallyExclusive group). Passing nullptr falls +/// back to the node's default group on every distro. #pragma once @@ -49,15 +55,21 @@ #include +#include + namespace ros2_medkit_gateway { namespace compat { using GenericServiceClient = rclcpp::GenericClient; -/// Create a GenericServiceClient (delegates to Node::create_generic_client) -inline GenericServiceClient::SharedPtr -create_generic_service_client(rclcpp::Node * node, const std::string & service_name, const std::string & service_type) { - return node->create_generic_client(service_name, service_type); +/// Create a GenericServiceClient (delegates to Node::create_generic_client). +/// @param group Callback group handling the reply callbacks; nullptr resolves +/// to the node's default group inside rclcpp. +inline GenericServiceClient::SharedPtr create_generic_service_client(rclcpp::Node * node, + const std::string & service_name, + const std::string & service_type, + rclcpp::CallbackGroup::SharedPtr group) { + return node->create_generic_client(service_name, service_type, rclcpp::ServicesQoS(), std::move(group)); } } // namespace compat @@ -333,17 +345,22 @@ class GenericServiceClient : public rclcpp::ClientBase { std::map pending_requests_; }; -/// Create a GenericServiceClient for Humble -inline GenericServiceClient::SharedPtr -create_generic_service_client(rclcpp::Node * node, const std::string & service_name, const std::string & service_type) { +/// Create a GenericServiceClient for Humble. +/// @param group Callback group handling the reply callbacks; nullptr resolves +/// to the node's default group inside NodeServices::add_client. +inline GenericServiceClient::SharedPtr create_generic_service_client(rclcpp::Node * node, + const std::string & service_name, + const std::string & service_type, + rclcpp::CallbackGroup::SharedPtr group) { rcl_client_options_t options = rcl_client_get_default_options(); auto client = std::make_shared( node->get_node_base_interface().get(), node->get_node_graph_interface(), service_name, service_type, options); - // Register the client with the node's default callback group so the executor - // polls it for incoming responses. Without this, handle_response() is never + // Register the client with the given callback group so the executor polls + // it for incoming responses. Without this, handle_response() is never // called and every future hangs until timeout. - node->get_node_services_interface()->add_client(std::dynamic_pointer_cast(client), nullptr); + node->get_node_services_interface()->add_client(std::dynamic_pointer_cast(client), + std::move(group)); return client; } diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/error_codes.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/error_codes.hpp index c27edbe3a..aa1dd4b1d 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/error_codes.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/error_codes.hpp @@ -147,6 +147,12 @@ constexpr const char * ERR_INSUFFICIENT_ACCESS_RIGHTS = "insufficient-access-rig /// SOVD standard: a required precondition was not fulfilled (409) constexpr const char * ERR_PRECONDITION_NOT_FULFILLED = "precondition-not-fulfilled"; +/// SOVD standard: entity queried but did not respond (504). SOVD reserves +/// 504 Gateway Timeout for "no response from the underlying entity in time" +/// - used when a ROS 2 round-trip times out and the outcome is therefore +/// UNKNOWN, as opposed to a definitive failure reported by the entity. +constexpr const char * ERR_NOT_RESPONDING = "not-responding"; + // Script error codes (vendor-specific only; generic cases use ERR_RESOURCE_NOT_FOUND / ERR_INVALID_PARAMETER) constexpr const char * ERR_SCRIPT_ALREADY_EXISTS = "x-medkit-script-already-exists"; constexpr const char * ERR_SCRIPT_MANAGED = "x-medkit-managed-script"; diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp index db385decd..a1e6c7242 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp @@ -14,9 +14,13 @@ #pragma once +#include +#include #include #include +#include "ros2_medkit_gateway/core/managers/operation_manager.hpp" +#include "ros2_medkit_gateway/core/operations/operation_types.hpp" #include "ros2_medkit_gateway/dto/operations.hpp" #include "ros2_medkit_gateway/http/handlers/handler_context.hpp" #include "ros2_medkit_gateway/http/response_types.hpp" @@ -25,6 +29,30 @@ namespace ros2_medkit_gateway { namespace handlers { +namespace detail { + +/// Failure shape produced by `map_cancel_result`. `std::nullopt` from the +/// mapper means "the cancellation is in progress" and the entry point should +/// render its success shape (204 for DELETE, 202 for PUT-stop). +struct CancelFailure { + int http_status; + const char * error_code; + std::string message; +}; + +/// Shared outcome mapping for the two cancel entry points (DELETE execution +/// and PUT-stop) - issue #576. Declared here rather than kept file-local so +/// the wire contract of every `CancelOutcome` is directly testable: the +/// manager-side guards (invalid uuid / goal evicted between the handler's +/// lookup and the manager's re-check) are only reachable over HTTP through a +/// race, so their mapping has to be pinned at this seam. +/// +/// @param verb "Cancel" or "Stop" - keeps each entry point's message wording. +std::optional map_cancel_result(const ActionCancelResult & result, OperationManager & operation_mgr, + const std::string & execution_id, const char * verb); + +} // namespace detail + /** * @brief Handlers for operation-related REST API endpoints (services and actions). * diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/managers/operation_manager.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/managers/operation_manager.hpp index b48fd522d..6b637b30e 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/managers/operation_manager.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/managers/operation_manager.hpp @@ -124,6 +124,16 @@ class OperationManager { const std::string & entity_id = ""); /// Cancel a running action goal. + /// + /// PRECONDITION: the caller has already resolved `goal_id` through + /// get_tracked_goal() and taken `action_path` from the resulting + /// ActionGoalInfo. Both HTTP entry points do exactly that and answer 404 + /// themselves when the lookup fails, so the only guard below that a + /// contract-respecting caller can trip is the tracked-goal re-check - and + /// only by racing the cleanup timer, which is why that one alone carries a + /// wire classification (CancelOutcome::kNotTracked -> 404). A malformed + /// goal_id cannot reach here at all: every key in the tracking map comes + /// from uuid_bytes_to_hex(). ActionCancelResult cancel_action_goal(const std::string & action_path, const std::string & goal_id); /// Get the result of a completed action. @@ -157,9 +167,26 @@ class OperationManager { /// the tracking map on the transport's executor thread. void subscribe_to_action_status(const std::string & action_path); - /// Unsubscribe from action status updates. Idempotent. + /// Unsubscribe from action status updates. Idempotent, and a no-op while + /// any goal for the path is still tracked. + /// + /// Postcondition (the property that matters, and the one that is testable): + /// on return, every path with a tracked goal has a live status stream. That + /// is stated as a postcondition rather than as atomicity on purpose - a goal + /// sent while the transport call is in flight does briefly race it, and this + /// method repairs that pair before returning instead of holding a lock + /// across rclcpp. A tracked goal without its status stream can never be + /// reconciled by the cancel-timeout path, so the repair is not optional. void unsubscribe_from_action_status(const std::string & action_path); + /// Resolved service / action call budget in seconds. Every service call and + /// every action RPC (send_goal, cancel_goal, get_result) is bounded by it, + /// so the value the gateway actually applies is observable rather than + /// merely configured. + int service_call_timeout_sec() const { + return service_call_timeout_sec_; + } + /// Test-only helper: inject a fully-formed ActionGoalInfo directly into the /// tracking map. Used by unit tests to exercise paths (e.g. stuck-goal /// eviction) without driving real action server traffic. Not part of the @@ -183,6 +210,11 @@ class OperationManager { void track_goal(const std::string & goal_id, const std::string & action_path, const std::string & action_type, const std::string & entity_id); + /// True while at least one goal for `action_path` is tracked. Cheaper than + /// get_goals_for_action (no copy, no sort) and safe to call while holding + /// subscriptions_mutex_ - nothing takes goals_mutex_ before that one. + bool has_goals_for_action(const std::string & action_path) const; + /// Propagate a goal status change observed via the action transport into /// the tracking map, firing the resource-change notifier on transitions. void on_status_callback(const std::string & action_path, const std::string & goal_id, ActionGoalStatus status); diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/operations/operation_types.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/operations/operation_types.hpp index e06a32407..db450476d 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/operations/operation_types.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/operations/operation_types.hpp @@ -53,10 +53,30 @@ struct ActionSendGoalResult { std::string error_message; }; +/// Outcome of a CancelGoal round-trip (issue #576). Distinguishes "the +/// server answered" from "no answer arrived in time": a timed-out cancel has +/// an UNKNOWN outcome (the request may well have been accepted) and must not +/// be reported as a definitive rejection. +enum class CancelOutcome : uint8_t { + kOk, ///< Server answered with return_code 0 (cancel accepted). + kTimeout, ///< No response within the budget - outcome unknown. + kServiceUnavailable, ///< cancel_goal service not discoverable (server gone). + kTransportError, ///< Null response / unknown type / exception / precondition failure. + kErrorResponse, ///< Server answered with return_code 1/2/3 (definitive). + /// The execution is not (or no longer) tracked - the cancel never left the + /// gateway. Reachable over HTTP when the cleanup timer evicts the goal + /// between the handler's lookup and the manager's own re-check; the + /// truthful answer is "no such execution", not "action server unavailable". + kNotTracked, +}; + /// Result of canceling an action goal. struct ActionCancelResult { - bool success; - int8_t return_code; ///< 0=accepted, 1=rejected, 2=unknown_id, 3=terminated + bool success = false; + int8_t return_code = 0; ///< 0=accepted, 1=rejected, 2=unknown_id, 3=terminated + /// Defaults to kTransportError so any early-return path that forgets to + /// classify itself reads as a transport failure, never as a rejection. + CancelOutcome outcome = CancelOutcome::kTransportError; std::string error_message; }; diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp index a4a0b2ada..a46a11bf5 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp @@ -62,6 +62,7 @@ #include "ros2_medkit_gateway/ros2/transports/ros2_topic_subscription_transport.hpp" #include "ros2_medkit_gateway/ros2/transports/ros2_topic_transport.hpp" #include "ros2_medkit_gateway/ros2/trigger_topic_subscriber.hpp" +#include "ros2_medkit_gateway/ros2_common/callback_groups.hpp" #include "ros2_medkit_gateway/trigger_fault_subscriber.hpp" namespace ros2_medkit_gateway { @@ -358,6 +359,13 @@ class GatewayNode : public rclcpp::Node { // manager and discovery side updates. std::shared_ptr topic_transport_; + // Shared callback groups for blocking-RPC response dispatch (Reentrant) + // and per-action status subscriptions (MutuallyExclusive) - issue #575. + // Declared BEFORE the transports so the groups destruct after every + // entity registered into them; the node itself only holds weak + // references to its callback groups. + ros2_common::GatewayCallbackGroups callback_groups_; + // Service / action transport adapters shared with OperationManager. Held // here so their lifetime matches the gateway's executor (transports own // rclcpp clients + subscriptions and must outlive the manager). diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/typed_router.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/typed_router.hpp index 81cfbe3b6..b29c728ac 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/typed_router.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/typed_router.hpp @@ -130,11 +130,17 @@ class TypedRequest { return req_.has_header("X-Medkit-No-Fan-Out"); } - /// Returns the request path (post-routing, post-prefix-strip). Handlers - /// occasionally need this to build a `Location` header for resources they - /// just created via POST (e.g. `Location: /`). This - /// is the only path-shaped read most handlers need; routes that need to - /// inspect path segments should use `path_param` instead. + /// Returns the request path exactly as the client sent it, **including the + /// `/api/v1` prefix**: routes are registered with the prefix already + /// concatenated (`RouteRegistry::register_all`) and nothing rewrites + /// `req.path` on the way in, so this is the absolute path of the resource + /// being addressed. + /// + /// That is what makes it the right basis for a `Location` header - either + /// verbatim (PUT, where the target IS the resource) or with the new id + /// appended (`Location: /` on POST). Do NOT wrap it in + /// `api_path()`: that would double the prefix. Routes that need to inspect + /// path segments should use `path_param` instead. const std::string & path() const { return req_.path; } diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp index 69b6d1411..c341bdeee 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp @@ -44,8 +44,20 @@ class Ros2ActionTransport : public ActionTransport { public: /** * @param node Non-owning ROS node used for client + subscription creation. + * @param rpc_group Callback group for the send_goal / get_result / + * cancel_goal clients' response callbacks - the shared Reentrant + * RPC group from ros2_common::create_gateway_callback_groups() + * (issue #575), so responses can be delivered while default-group + * callbacks run. + * @param status_group Callback group for the `/_action/status` + * subscriptions - the shared MutuallyExclusive group, preserving + * per-subscription in-order delivery while decoupling status + * tracking from the default group. + * Both group shared_ptrs are kept alive by this transport (the + * node only holds weak references to its callback groups). */ - explicit Ros2ActionTransport(rclcpp::Node * node); + Ros2ActionTransport(rclcpp::Node * node, rclcpp::CallbackGroup::SharedPtr rpc_group, + rclcpp::CallbackGroup::SharedPtr status_group); ~Ros2ActionTransport() override; @@ -81,6 +93,11 @@ class Ros2ActionTransport : public ActionTransport { void on_status_msg(const std::string & action_path, const action_msgs::msg::GoalStatusArray::ConstSharedPtr & msg); rclcpp::Node * node_; + /// Shared Reentrant group for RPC response dispatch and shared + /// MutuallyExclusive group for status subscriptions; owned here because + /// the node keeps only weak references to its callback groups. + rclcpp::CallbackGroup::SharedPtr rpc_group_; + rclcpp::CallbackGroup::SharedPtr status_group_; std::shared_ptr serializer_; /// Set on the first shutdown signal so callbacks short-circuit while the diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_service_transport.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_service_transport.hpp index c61936b57..64ab757c5 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_service_transport.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/transports/ros2_service_transport.hpp @@ -38,8 +38,14 @@ class Ros2ServiceTransport : public ServiceTransport { public: /** * @param node Non-owning ROS node used for client creation. + * @param rpc_group Callback group the clients' response callbacks are + * dispatched on - the shared Reentrant RPC group from + * ros2_common::create_gateway_callback_groups() (issue #575), so a + * response can be delivered while default-group callbacks run. The + * transport keeps the shared_ptr alive for its own lifetime (the + * node only holds a weak reference to the group). */ - explicit Ros2ServiceTransport(rclcpp::Node * node); + Ros2ServiceTransport(rclcpp::Node * node, rclcpp::CallbackGroup::SharedPtr rpc_group); ~Ros2ServiceTransport() override; @@ -58,6 +64,9 @@ class Ros2ServiceTransport : public ServiceTransport { const std::string & service_type); rclcpp::Node * node_; + /// Shared Reentrant group for response dispatch; owned here because the + /// node keeps only a weak reference to its callback groups. + rclcpp::CallbackGroup::SharedPtr rpc_group_; std::shared_ptr serializer_; mutable std::shared_mutex clients_mutex_; diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2_common/callback_groups.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2_common/callback_groups.hpp new file mode 100644 index 000000000..12fbdac59 --- /dev/null +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2_common/callback_groups.hpp @@ -0,0 +1,75 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include + +namespace ros2_medkit_gateway::ros2_common { + +/** + * @brief Callback groups for the gateway node's blocking-RPC clients and + * action-status subscriptions (issue #575). + * + * WHY THIS LIVES IN ros2_common/: creating callback groups is one of the + * node-mutating rclcpp calls covered by the issue-#375 regression gate + * (`scripts/check_no_naked_subscriptions.sh` bans `create_callback_group` + * outside `ros2_common/`). Group creation is therefore funnelled through + * this factory instead of being scattered across transports. + * + * Thread-safety contract: + * - `create_gateway_callback_groups()` is called exactly ONCE, on the single + * startup thread, before the executor spins and before any RESTServer + * worker exists - group creation itself never races other node mutations. + * - The returned shared_ptrs are handed to the transports as constructor + * dependencies and may afterwards be passed to entity-creation calls + * (`create_generic_client` / `create_subscription`) from HTTP worker + * threads. Re-homing those entities into these groups adds no new + * concurrency: registration into a shared group is the same class of + * operation as the pre-existing registration into the shared *default* + * group, and stays serialized by the per-transport client mutexes. + * - The transports keep their group shared_ptr for their own lifetime; the + * node only holds weak references to its groups, so dropping the last + * shared_ptr would silently stop dispatch for every entity in the group. + */ +struct GatewayCallbackGroups { + /// Shared Reentrant group for the response callbacks of blocking RPC + /// clients: the generic service clients behind `/operations` executions + /// and the per-action send_goal / get_result / cancel_goal client trio. + /// Reentrant so a response can be dispatched on any executor thread even + /// while a default-group callback (e.g. a discovery refresh pass) is + /// running - this is what makes `server.executor_threads` > 1 buy actual + /// RPC-response parallelism. A Reentrant group does not *require* a + /// second thread; a single-threaded executor still services it. + rclcpp::CallbackGroup::SharedPtr rpc_reentrant; + + /// Shared MutuallyExclusive group for the per-action `/_action/status` + /// subscriptions. Mutually exclusive so GoalStatusArray callbacks keep + /// their in-order delivery (status transitions must not be observed out + /// of order), while being decoupled from the default group so a long + /// refresh pass cannot delay goal-status tracking. + rclcpp::CallbackGroup::SharedPtr action_status; +}; + +/** + * @brief Create the gateway's shared callback groups on @p node. + * + * Must be called once, from the startup thread, before the node is added to + * a spinning executor (see the thread-safety contract above). Both groups + * are created with `automatically_add_to_executor_with_node = true`, so + * whichever executor the node is added to dispatches them. + */ +GatewayCallbackGroups create_gateway_callback_groups(rclcpp::Node & node); + +} // namespace ros2_medkit_gateway::ros2_common diff --git a/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp index bc916e284..b6e3a5279 100644 --- a/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp @@ -297,45 +297,48 @@ ActionSendGoalResult OperationManager::send_component_action_goal(const std::str return send_action_goal(action_path, resolved_type, goal, entity_id); } -namespace { -/// Floor for the CancelGoal budget. Cancel is a service call to the action server plus that -/// server's own handling, so it is bounded by the SERVER, not by us; a very small configured -/// service timeout would otherwise report a live cancel as a failure. The configured -/// service_call_timeout_sec still wins whenever it is larger, so lowering it keeps bounding -/// cancel like every other call in this file. -constexpr double kCancelGoalFloorSec = 15.0; -} // namespace - ActionCancelResult OperationManager::cancel_action_goal(const std::string & action_path, const std::string & goal_id) { ActionCancelResult result; result.success = false; result.return_code = 0; + // Defence in depth only: per this method's documented precondition every + // goal_id in tracked_goals_ came from uuid_bytes_to_hex, so a caller that + // resolved the goal first cannot reach this branch. Left unclassified + // (kTransportError) deliberately - it has no wire contract to honour. if (!is_valid_uuid_hex(goal_id)) { result.error_message = "Invalid goal_id format: must be 32 hex characters"; return result; } + // This one IS reachable through the contract: the caller resolved the goal, + // then the cleanup timer evicted it before this re-check. Answering + // "action server unavailable" would blame a healthy server for what is + // simply an execution that no longer exists. auto goal_info = get_tracked_goal(goal_id); if (!goal_info) { + result.outcome = CancelOutcome::kNotTracked; result.error_message = "Unknown goal_id - not tracked"; return result; } + // Also defence in depth: the transport is a constructor dependency and the + // gateway always supplies one. A missing transport genuinely is a + // gateway-side transport failure, so the default classification fits. if (!action_transport_) { result.error_message = "ActionTransport not configured"; return result; } - // A CancelGoal round-trip is a service call to the action server plus that server's own - // handling, so it is bounded by the SERVER, not by us. 5s was enough on an idle machine and - // not on a loaded one: under a busy CI container the request timed out and the cancel was - // reported as a vendor error while the goal had in fact been accepted for cancellation. - const auto cancel_timeout = - std::chrono::duration(std::max(static_cast(service_call_timeout_sec_), kCancelGoalFloorSec)); - result = action_transport_->cancel_goal(action_path, goal_id, cancel_timeout); + // Cancel gets the same budget as every other action RPC. A timed-out + // cancel is no longer conflated with a rejection (issue #576): the + // transport reports it as CancelOutcome::kTimeout and the HTTP layer + // reconciles it against the /_action/status stream, so there is no need + // for the old 15s floor that papered over slow dispatch. + result = + action_transport_->cancel_goal(action_path, goal_id, std::chrono::duration(service_call_timeout_sec_)); - if (result.success && result.return_code == 0) { + if (result.outcome == CancelOutcome::kOk) { update_goal_status(goal_id, ActionGoalStatus::CANCELING); } return result; @@ -402,6 +405,13 @@ std::vector OperationManager::get_goals_for_action(const std::st return goals; } +bool OperationManager::has_goals_for_action(const std::string & action_path) const { + std::lock_guard lock(goals_mutex_); + return std::any_of(tracked_goals_.begin(), tracked_goals_.end(), [&action_path](const auto & entry) { + return entry.second.action_path == action_path; + }); +} + std::optional OperationManager::get_latest_goal_for_action(const std::string & action_path) const { auto goals = get_goals_for_action(action_path); if (goals.empty()) { @@ -413,10 +423,25 @@ std::optional OperationManager::get_latest_goal_for_action(const void OperationManager::update_goal_status(const std::string & goal_id, ActionGoalStatus status) { std::lock_guard lock(goals_mutex_); auto it = tracked_goals_.find(goal_id); - if (it != tracked_goals_.end()) { - it->second.status = status; - it->second.last_update = std::chrono::system_clock::now(); + if (it == tracked_goals_.end()) { + return; } + // A terminal state is final in the ROS 2 action protocol, and every caller + // of this method is an RPC completion (goal accepted -> EXECUTING, cancel + // accepted -> CANCELING, get_result -> reported status) that can land after + // the /_action/status stream has already delivered the terminal state. The + // guard lives here rather than at the cancel call site because the hazard + // is the same for all three: nothing ever corrects a backwards write - the + // server publishes no further status frame, so the goal would be reported + // as still running until the stuck-goal path force-evicts it with a false + // "action server crashed" warning. The stream stays the authority; it + // writes directly under this lock in on_status_callback and is deliberately + // not routed through here. + if (is_terminal_status(it->second.status)) { + return; + } + it->second.status = status; + it->second.last_update = std::chrono::system_clock::now(); } void OperationManager::update_goal_feedback(const std::string & goal_id, const json & feedback) { @@ -501,15 +526,47 @@ void OperationManager::unsubscribe_from_action_status(const std::string & action bool was_subscribed = false; { std::lock_guard lock(subscriptions_mutex_); + // cleanup_old_goals reads the goal count first and unsubscribes after, so + // a goal sent in between would keep a subscription that is about to be + // destroyed: tracked, but with no status stream. Since #576 the stream + // decides 204-vs-504 for a timed-out cancel, so re-check under the same + // lock that guards the erase and let a live goal veto the unsubscribe. + if (has_goals_for_action(action_path)) { + return; + } auto it = subscribed_paths_.find(action_path); if (it != subscribed_paths_.end()) { subscribed_paths_.erase(it); was_subscribed = true; } } - if (was_subscribed && action_transport_) { - action_transport_->unsubscribe_status(action_path); + if (!was_subscribed || !action_transport_) { + return; + } + action_transport_->unsubscribe_status(action_path); + + // The veto above only covers the decision, not the transport call: this + // runs with no lock held, so a goal sent meanwhile has already re-flagged + // the path and received a no-op from the transport (whose subscription was + // still alive at that instant) - and the call just above then destroyed it. + // The flag would keep subscribe_to_action_status short-circuiting forever, + // so repair the pair here instead: clear the flag and subscribe again. + // + // Deliberately NOT solved by holding subscriptions_mutex_ across the + // transport call. That would add a lock-order edge from our mutex into + // rclcpp's subscription create/destroy path, which neither this method nor + // subscribe_to_action_status has today. The residual window is one transport + // call wide and self-healing, and the status stream is TRANSIENT_LOCAL, so + // the fresh subscription is delivered the goal's latest status sample on + // match rather than waiting for the next publish. + if (!has_goals_for_action(action_path)) { + return; + } + { + std::lock_guard lock(subscriptions_mutex_); + subscribed_paths_.erase(action_path); } + subscribe_to_action_status(action_path); } void OperationManager::on_status_callback(const std::string & action_path, const std::string & goal_id, diff --git a/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp b/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp index 2e7df83ee..fe060826c 100644 --- a/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp +++ b/src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp @@ -61,7 +61,7 @@ EntityFreezeFrameCapture::EntityFreezeFrameCapture(rclcpp::Node * node, ros2_com // pre-match window instead. auto slot = ros2_common::Ros2SubscriptionSlot::create_typed( exec, fault_events_topic, rclcpp::QoS(100).reliable(), - [this](std::shared_ptr msg) { + [this](const std::shared_ptr & msg) { on_fault_event(msg); }); if (!slot) { diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 03c6ea9f7..08e230c59 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -55,14 +55,20 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki // instead of scaling with the host core count, so the footprint stays the // same on a 4-core SBC and a 64-core server. // - // executor_threads: the main executor only delivers the node's own callbacks + // executor_threads: the main executor delivers the node's own callbacks // (timers, graph events, subscriptions) and the service-response callbacks - // that complete operation/action RPC futures. All of these run on the node's - // default callback group, which is mutually-exclusive, so they serialize - // through a single thread regardless of this count - raising it buys no - // RPC-response parallelism. The reason a small executor is safe is solely that - // the blocking wait for an RPC runs on the cpp-httplib pool thread (off the - // executor), never on an executor thread, so it cannot deadlock the executor. + // that complete operation/action RPC futures. The RPC response callbacks + // live in a shared Reentrant callback group (issue #575; see + // ros2_common/callback_groups.hpp), so a second executor thread can + // dispatch a response while a default-group callback (e.g. 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 default MutuallyExclusive group on purpose: + // refresh passes must stay serialized and those subscriptions rely on + // in-order delivery. A single 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 (off the executor), never on an executor + // thread, so it cannot deadlock the executor. // // http_thread_pool_size: each active SSE stream pins one worker for its // lifetime and each cold-/data wait parks one for up to @@ -590,10 +596,32 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki const auto topic_sample_timeout_sec = get_parameter("topic_sample_timeout_sec").as_double(); topic_transport_ = std::make_shared(this, topic_sample_timeout_sec); data_access_mgr_ = std::make_unique(topic_transport_, topic_sample_timeout_sec); - service_transport_ = std::make_shared(this); - action_transport_ = std::make_shared(this); - const auto service_call_timeout_sec = - static_cast(declare_parameter("service_call_timeout_sec", static_cast(10))); + // Shared callback groups for blocking-RPC response dispatch and action + // status tracking (issue #575). Created once, here on the startup thread, + // before any executor spins this node and before RESTServer threads exist + // (see ros2_common/callback_groups.hpp for the thread-safety contract). + callback_groups_ = ros2_common::create_gateway_callback_groups(*this); + service_transport_ = std::make_shared(this, callback_groups_.rpc_reentrant); + action_transport_ = + std::make_shared(this, callback_groups_.rpc_reentrant, callback_groups_.action_status); + // Budget for every service call and every action RPC, including the action + // cancel (issue #576 removed the special 15s cancel floor, making this the + // sole cancel budget). Degenerate values are not cosmetic here: 0 or a + // negative makes the response wait expire immediately, so every cancel + // answers 504 unless the status stream wins the race, and an unbounded + // value pins an HTTP worker for as long as it says. Clamp to the documented + // range with a warning, like the sibling timeouts. + static constexpr int64_t kMinServiceCallTimeoutSec = 1; + static constexpr int64_t kMaxServiceCallTimeoutSec = 3600; + const auto raw_service_call_timeout = + declare_parameter("service_call_timeout_sec", static_cast(10)); + const auto clamped_service_call_timeout = + std::clamp(raw_service_call_timeout, kMinServiceCallTimeoutSec, kMaxServiceCallTimeoutSec); + if (clamped_service_call_timeout != raw_service_call_timeout) { + RCLCPP_WARN(get_logger(), "service_call_timeout_sec %" PRId64 " clamped to %" PRId64, raw_service_call_timeout, + clamped_service_call_timeout); + } + const auto service_call_timeout_sec = static_cast(clamped_service_call_timeout); operation_mgr_ = std::make_unique(service_transport_, action_transport_, discovery_mgr_.get(), service_call_timeout_sec); diff --git a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp index 5f535c8ba..2c244700c 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -205,6 +205,92 @@ ErrorInfo make_provider_error(const OperationProviderErrorInfo & info, const std } // namespace +namespace detail { + +/// Shared outcome mapping for the two cancel entry points (DELETE execution +/// and PUT-stop) - issue #576: +/// - kOk: success. +/// - kTimeout: the CancelGoal response did not arrive, so the outcome is +/// UNKNOWN, not a rejection. Reconcile against the tracked goal fed by the +/// /_action/status stream: if the stream already shows CANCELING/CANCELED +/// the cancellation is in fact happening -> success. Otherwise 504 + +/// standard `not-responding` (SOVD: "no response from the underlying +/// entity in time"). The tracked status is NOT written on this path - the +/// status stream stays the authority. +/// - kServiceUnavailable: 503 - the action server is gone; retry may help. +/// - kTransportError: 500 - the request could not be delivered/parsed. +/// - kErrorResponse: 400 + `x-medkit-ros2-action-rejected` - the server +/// answered and definitively refused (return_code 1/2/3). This function is +/// the ONLY place that words those three codes: the transport deliberately +/// keeps no second copy, because only the HTTP layer knows whether the +/// client asked to cancel or to stop, and two hand-maintained tables for +/// the same protocol constants drift silently. +/// - kNotTracked: 404 - the execution no longer exists (evicted between the +/// handler's lookup and the manager's re-check); no request ever reached +/// the action server, so an availability code would misdirect the operator. +/// +/// @param verb "Cancel" or "Stop" - keeps each entry point's message wording. +std::optional map_cancel_result(const ActionCancelResult & result, OperationManager & operation_mgr, + const std::string & execution_id, const char * verb) { + switch (result.outcome) { + case CancelOutcome::kOk: + return std::nullopt; + case CancelOutcome::kTimeout: { + auto tracked = operation_mgr.get_tracked_goal(execution_id); + if (tracked.has_value() && + (tracked->status == ActionGoalStatus::CANCELING || tracked->status == ActionGoalStatus::CANCELED)) { + return std::nullopt; + } + std::string message = + std::string(verb) + " outcome unknown: the action server did not answer the cancel request in time. "; + // CANCELED already reconciled above, so a terminal status here means + // the goal finished on its own. Telling the client to watch for + // progress would describe something that cannot happen - say what the + // gateway already knows instead, in the vocabulary the resource being + // named actually answers in (`sovd_status_from_ros2`, not the raw ROS + // word: the execution resource never emits "succeeded"/"aborted"). + if (tracked.has_value() && + (tracked->status == ActionGoalStatus::SUCCEEDED || tracked->status == ActionGoalStatus::ABORTED)) { + message += "The execution status resource already reports the goal as " + + sovd_status_from_ros2(tracked->status) + ", so there is nothing left to cancel."; + } else { + // `status` alone cannot express the answer the client is asking for - + // it renders CANCELED and ABORTED identically as "failed" - so name + // the field that can. + message += "Poll the execution status resource and read x-medkit.ros2_status to learn the goal's outcome."; + } + return CancelFailure{504, ERR_NOT_RESPONDING, std::move(message)}; + } + case CancelOutcome::kServiceUnavailable: + return CancelFailure{503, ERR_X_MEDKIT_ROS2_ACTION_UNAVAILABLE, + result.error_message.empty() ? "Cancel service not available" : result.error_message}; + case CancelOutcome::kTransportError: + return CancelFailure{500, ERR_X_MEDKIT_ROS2_ACTION_UNAVAILABLE, + result.error_message.empty() ? std::string(verb) + " failed" : result.error_message}; + case CancelOutcome::kNotTracked: + return CancelFailure{404, ERR_RESOURCE_NOT_FOUND, "Execution not found"}; + case CancelOutcome::kErrorResponse: + break; + } + std::string message; + switch (result.return_code) { + case 1: + message = std::string(verb) + " request rejected"; + break; + case 2: + message = "Unknown execution ID"; + break; + case 3: + message = "Execution already terminated"; + break; + default: + message = result.error_message.empty() ? std::string(verb) + " failed" : result.error_message; + } + return CancelFailure{400, ERR_X_MEDKIT_ROS2_ACTION_REJECTED, std::move(message)}; +} + +} // namespace detail + // ============================================================================= // GET /{entity}/operations - list operations // ============================================================================= @@ -542,9 +628,12 @@ OperationHandlers::create_execution(const http::TypedRequest & req, dto::Executi async_dto.id = action_result.goal_id; async_dto.status = "running"; - const std::string base_path = (lookup->entity_type == "app") ? "/api/v1/apps/" : "/api/v1/components/"; - const std::string location = - base_path + entity_id + "/operations/" + operation_id + "/executions/" + action_result.goal_id; + // The created execution is a sub-resource of the collection this POST + // targeted, so append the new id to the request path. Hand-building the + // prefix from an apps/components pair points areas and functions + // clients - the route is registered for all four entity types - into + // the components collection, and bypasses api_path() besides. + const std::string location = req.path() + "/" + action_result.goal_id; http::ResponseAttachments att; att.with_header("Location", location); @@ -758,28 +847,19 @@ http::Result OperationHandlers::cancel_execution(const http::Ty } auto result = operation_mgr->cancel_action_goal(goal_info->action_path, execution_id); - if (result.success && result.return_code == 0) { + auto failure = detail::map_cancel_result(result, *operation_mgr, execution_id, "Cancel"); + if (!failure.has_value()) { return http::NoContent{}; } - std::string error_msg; - switch (result.return_code) { - case 1: - error_msg = "Cancel request rejected"; - break; - case 2: - error_msg = "Unknown execution ID"; - break; - case 3: - error_msg = "Execution already terminated"; - break; - default: - error_msg = result.error_message.empty() ? "Cancel failed" : result.error_message; + json params{{"entity_id", entity_id}, {"operation_id", operation_id}, {"execution_id", execution_id}}; + // `return_code` only means something when the action server actually + // answered; carrying a hardcoded 0 on the timeout / unavailable / guard + // paths is exactly the ambiguity issue #576 is about. + if (result.outcome == CancelOutcome::kErrorResponse) { + params["return_code"] = result.return_code; } - return tl::make_unexpected(make_error(400, ERR_X_MEDKIT_ROS2_ACTION_REJECTED, error_msg, - json{{"entity_id", entity_id}, - {"operation_id", operation_id}, - {"execution_id", execution_id}, - {"return_code", result.return_code}})); + return tl::make_unexpected( + make_error(failure->http_status, failure->error_code, failure->message, std::move(params))); } // ============================================================================= @@ -833,43 +913,54 @@ OperationHandlers::update_execution(const http::TypedRequest & req, const dto::E // supported_capabilities hint. if (capability == "stop") { auto result = operation_mgr->cancel_action_goal(goal_info->action_path, execution_id); - if (result.success && result.return_code == 0) { - const std::string base_path = - req.path().find("/apps/") != std::string::npos ? "/api/v1/apps/" : "/api/v1/components/"; - const std::string location = - base_path + entity_id + "/operations/" + operation_id + "/executions/" + execution_id; + auto failure = detail::map_cancel_result(result, *operation_mgr, execution_id, "Stop"); + if (!failure.has_value()) { + // The execution resource IS the request target for PUT, so echo the + // requested path: the route is registered for apps, components, areas + // and functions alike, and a hand-built apps/components pair sends an + // areas client into the wrong collection. + const std::string & location = req.path(); + + // Render the tracked status rather than assuming "running": the + // reconcile set includes CANCELED, which GET reports as "failed", and + // a 202 body must not contradict the resource Location points at. + // + // If the goal is gone by now - the cleanup timer can evict it between + // the mapper's read and this one - then there is no status to render + // and the Location we would hand out answers 404. Say that instead of + // inventing "running", which would reproduce exactly the contradiction + // this branch removed. + auto tracked = operation_mgr->get_tracked_goal(execution_id); + if (!tracked.has_value()) { + return tl::make_unexpected( + make_error(404, ERR_RESOURCE_NOT_FOUND, "Execution not found", + json{{"entity_id", entity_id}, {"operation_id", operation_id}, {"execution_id", execution_id}})); + } dto::OperationExecution exec_dto; exec_dto.id = execution_id; - exec_dto.status = "running"; // canceling is still "running" in SOVD terms + exec_dto.status = sovd_status_from_ros2(tracked->status); http::ResponseAttachments att; att.with_status(202).with_header("Location", location); return SuccessPair{std::move(exec_dto), std::move(att)}; } - std::string error_msg; - switch (result.return_code) { - case 1: - error_msg = "Stop request rejected"; - break; - case 2: - error_msg = "Unknown execution ID"; - break; - case 3: - error_msg = "Execution already terminated"; - break; - default: - error_msg = result.error_message.empty() ? "Stop failed" : result.error_message; + json params{{"entity_id", entity_id}, + {"operation_id", operation_id}, + {"execution_id", execution_id}, + {"capability", capability}}; + if (result.outcome == CancelOutcome::kErrorResponse) { + params["return_code"] = result.return_code; } - return tl::make_unexpected(make_error(400, ERR_X_MEDKIT_ROS2_ACTION_REJECTED, error_msg, - json{{"entity_id", entity_id}, - {"operation_id", operation_id}, - {"execution_id", execution_id}, - {"capability", capability}})); + return tl::make_unexpected( + make_error(failure->http_status, failure->error_code, failure->message, std::move(params))); } if (capability == "execute") { + // `precondition-not-fulfilled` is the SOVD standard code for "prerequisites + // not met" and is what the gateway already answers its lifecycle 409 with; + // `invalid-request` is not in the standard code list at all. return tl::make_unexpected( - make_error(409, ERR_INVALID_REQUEST, + make_error(409, ERR_PRECONDITION_NOT_FULFILLED, "Cannot re-execute while operation is running. Cancel first, then start new execution.", json{{"entity_id", entity_id}, {"operation_id", operation_id}, diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index 958b8b67b..fd56e8a9c 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -695,6 +695,16 @@ void RESTServer::setup_routes() { .summary(std::string("Update execution for ") + et.singular) .description("Sends a control command to a running execution.") .response(202, "Accepted (asynchronous control)", SB::ref("OperationExecution")) + // 400/404/500 come from the registry's automatic response-level + // GenericError $ref; the remaining statuses this route can return + // need manual declarations, or the generated SDK has no branch for + // them (issue #576). + .response(409, "Execution is still running (precondition-not-fulfilled)", + nlohmann::json{{"$ref", "#/components/schemas/GenericError"}}) + .response(503, "Action server unavailable (x-medkit-ros2-action-unavailable)", + nlohmann::json{{"$ref", "#/components/schemas/GenericError"}}) + .response(504, "No cancel response in time - outcome unknown (not-responding)", + nlohmann::json{{"$ref", "#/components/schemas/GenericError"}}) .operation_id(std::string("update") + capitalize(et.singular) + "Execution"); reg.del(entity_path + "/operations/{operation_id}/executions/{execution_id}", @@ -704,6 +714,12 @@ void RESTServer::setup_routes() { .tag("Operations") .summary(std::string("Cancel execution for ") + et.singular) .description("Cancels a running execution.") + // Same cancel-outcome statuses as PUT-stop (issue #576); + // 400/404/500 are auto-declared by the registry. + .response(503, "Action server unavailable (x-medkit-ros2-action-unavailable)", + nlohmann::json{{"$ref", "#/components/schemas/GenericError"}}) + .response(504, "No cancel response in time - outcome unknown (not-responding)", + nlohmann::json{{"$ref", "#/components/schemas/GenericError"}}) .operation_id(std::string("cancel") + capitalize(et.singular) + "Execution"); // --- Configurations --- diff --git a/src/ros2_medkit_gateway/src/main.cpp b/src/ros2_medkit_gateway/src/main.cpp index e795e909a..128e810a4 100644 --- a/src/ros2_medkit_gateway/src/main.cpp +++ b/src/ros2_medkit_gateway/src/main.cpp @@ -76,19 +76,23 @@ int main(int argc, char ** argv) { // rclcpp's default (host cores, minimum 2), so the footprint does not grow // with the host core count. // - // Note this count does NOT buy RPC-response parallelism: the futures behind - // operation/action RPCs are completed by service-response callbacks, and - // every client here registers on the node's default callback group, which is - // mutually-exclusive - so those responses, timers, and graph events all - // serialize through a single executor thread no matter how high this is set. - // The reason a small executor is safe is solely that 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 additionally uses its own private executor. Raise this only if - // the node's own callback load (e.g. very frequent graph churn) grows. The - // Ros2SubscriptionExecutor built below owns its own internal single-threaded - // executor (spun from its worker thread); the subscription node is - // intentionally not added here. + // What the count buys (issue #575): the response callbacks of the blocking + // RPC clients (generic service clients + the per-action client trio) live + // in a shared Reentrant callback group (ros2_common/callback_groups.hpp), + // so with 2+ threads a service/action response is dispatched even while a + // default-group callback (e.g. a discovery refresh pass) is running. The + // node's timers, graph events and the SSE-fault / trigger-fault / rosout + // subscriptions stay in the default MutuallyExclusive group by design - + // refresh passes must stay serialized and those subscriptions rely on + // in-order delivery; per-action status subscriptions sit in their own + // MutuallyExclusive group, ordered but decoupled from the default group. + // A single thread is still 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 additionally + // uses its own private executor. The Ros2SubscriptionExecutor built below + // owns its own internal single-threaded executor (spun from its worker + // thread); the subscription node is intentionally not added here. const auto executor_threads = ros2_medkit_gateway::clamp_thread_count(node->get_parameter("server.executor_threads").as_int(), 1, 256); rclcpp::executors::MultiThreadedExecutor executor(rclcpp::ExecutorOptions(), executor_threads); diff --git a/src/ros2_medkit_gateway/src/ros2/transports/ros2_action_transport.cpp b/src/ros2_medkit_gateway/src/ros2/transports/ros2_action_transport.cpp index 5a5987474..5dff4bed2 100644 --- a/src/ros2_medkit_gateway/src/ros2/transports/ros2_action_transport.cpp +++ b/src/ros2_medkit_gateway/src/ros2/transports/ros2_action_transport.cpp @@ -103,8 +103,12 @@ ActionGoalStatus from_status_byte(int8_t status) { } // namespace -Ros2ActionTransport::Ros2ActionTransport(rclcpp::Node * node) - : node_(node), serializer_(std::make_shared()) { +Ros2ActionTransport::Ros2ActionTransport(rclcpp::Node * node, rclcpp::CallbackGroup::SharedPtr rpc_group, + rclcpp::CallbackGroup::SharedPtr status_group) + : node_(node) + , rpc_group_(std::move(rpc_group)) + , status_group_(std::move(status_group)) + , serializer_(std::make_shared()) { RCLCPP_INFO(node_->get_logger(), "Ros2ActionTransport initialised (native serialization)"); } @@ -143,15 +147,17 @@ Ros2ActionTransport::ActionClientSet & Ros2ActionTransport::get_or_create_client std::string send_goal_service = action_path + "/_action/send_goal"; std::string send_goal_type = ServiceActionTypes::get_action_send_goal_service_type(action_type); - clients.send_goal_client = compat::create_generic_service_client(node_, send_goal_service, send_goal_type); + clients.send_goal_client = + compat::create_generic_service_client(node_, send_goal_service, send_goal_type, rpc_group_); std::string get_result_service = action_path + "/_action/get_result"; std::string get_result_type = ServiceActionTypes::get_action_get_result_service_type(action_type); - clients.get_result_client = compat::create_generic_service_client(node_, get_result_service, get_result_type); + clients.get_result_client = + compat::create_generic_service_client(node_, get_result_service, get_result_type, rpc_group_); std::string cancel_service = action_path + "/_action/cancel_goal"; clients.cancel_goal_client = - compat::create_generic_service_client(node_, cancel_service, "action_msgs/srv/CancelGoal"); + compat::create_generic_service_client(node_, cancel_service, "action_msgs/srv/CancelGoal", rpc_group_); RCLCPP_DEBUG(node_->get_logger(), "Created action clients for %s (type: %s)", action_path.c_str(), action_type.c_str()); @@ -249,6 +255,7 @@ ActionCancelResult Ros2ActionTransport::cancel_goal(const std::string & action_p ActionCancelResult result; result.success = false; result.return_code = 0; + result.outcome = CancelOutcome::kTransportError; try { // Cancel does not need an action_type to be valid; reuse cached clients @@ -277,6 +284,7 @@ ActionCancelResult Ros2ActionTransport::cancel_goal(const std::string & action_p std::chrono::milliseconds{static_cast(std::max(timeout.count(), 0.0) * 1000.0)}; if (!clients.cancel_goal_client->wait_for_service(std::chrono::seconds(2))) { + result.outcome = CancelOutcome::kServiceUnavailable; result.error_message = "Cancel service not available"; return result; } @@ -294,6 +302,10 @@ ActionCancelResult Ros2ActionTransport::cancel_goal(const std::string & action_p if (future_status != std::future_status::ready) { clients.cancel_goal_client->remove_pending_request(future_and_id.request_id); ros2_medkit_serialization::destroy_ros_message(&ros_request); + // The response did not arrive in time: the cancel may well have been + // accepted server-side, so the outcome is UNKNOWN - callers must not + // treat this as a rejection (issue #576). + result.outcome = CancelOutcome::kTimeout; result.error_message = "Cancel request timed out"; return result; } @@ -315,24 +327,20 @@ ActionCancelResult Ros2ActionTransport::cancel_goal(const std::string & action_p result.success = true; result.return_code = static_cast(response.value("return_code", 0)); + result.outcome = result.return_code == 0 ? CancelOutcome::kOk : CancelOutcome::kErrorResponse; if (result.return_code == 0) { RCLCPP_INFO(node_->get_logger(), "Cancel request accepted for goal: %s", goal_id.c_str()); } else { - switch (result.return_code) { - case 1: - result.error_message = "Cancel request rejected"; - break; - case 2: - result.error_message = "Unknown goal ID"; - break; - case 3: - result.error_message = "Goal already terminated"; - break; - default: - result.error_message = "Unknown cancel error"; - break; - } + // Deliberately NOT a per-return_code message table: the client-facing + // wording for CancelGoal's documented codes 1-3 lives in exactly one + // place, `handlers::detail::map_cancel_result`, which alone knows + // whether the client asked to cancel or to stop. A second copy here + // would let a deleted mapper case silently change the wire message + // with every test still green. What remains is the fallback for a + // code the protocol does not define. + result.error_message = + "Cancel rejected by action server (return_code " + std::to_string(result.return_code) + ")"; } } catch (const std::exception & e) { @@ -440,8 +448,34 @@ void Ros2ActionTransport::subscribe_status(const std::string & action_path, Stat on_status_msg(action_path, msg); }; - auto subscription = - node_->create_subscription(status_topic, rclcpp::QoS(10).best_effort(), cb); + // Dispatch on the shared MutuallyExclusive status group (issue #575): + // in-order delivery per subscription is preserved, but status tracking is + // no longer serialized behind the node's default group. + rclcpp::SubscriptionOptions sub_options; + sub_options.callback_group = status_group_; + // Match the action protocol's own status profile: rcl_action declares + // KEEP_LAST(1) + RELIABLE + TRANSIENT_LOCAL + // (rcl_action/default_qos.h, rcl_action_qos_profile_status_default) for the + // server's publisher AND the client's subscription alike. + // + // Durability is the load-bearing part. The gateway subscribes only after + // the goal has been sent, so on the first goal for a path the action can + // reach a terminal state while this subscription is still matching. A + // VOLATILE reader is delivered nothing on match and the terminal frame is + // lost for good - no other code path re-reads a goal's status - which since + // #576 also means a timed-out cancel can never reconcile to 204. A + // TRANSIENT_LOCAL reader receives the writer's last sample on match, which + // is precisely the frame it missed. + // + // Cost of the reliable reader: with KEEP_LAST(1) the publisher overwrites + // rather than blocking, so a slow gateway cannot stall an action server + // indefinitely - the exposure is bounded by the writer's max_blocking_time + // plus retransmission traffic. Depth 1 can coalesce intermediate + // transitions (EXECUTING -> CANCELING -> CANCELED may arrive as CANCELED + // only); harmless here, because the tracking map stores the goal's current + // status and the cancel reconciliation accepts CANCELING or CANCELED. + auto subscription = node_->create_subscription( + status_topic, rclcpp::QoS(rclcpp::KeepLast(1)).reliable().transient_local(), cb, sub_options); status_subscriptions_[action_path] = subscription; RCLCPP_INFO(node_->get_logger(), "Subscribed to action status: %s", status_topic.c_str()); diff --git a/src/ros2_medkit_gateway/src/ros2/transports/ros2_service_transport.cpp b/src/ros2_medkit_gateway/src/ros2/transports/ros2_service_transport.cpp index 859600a44..5accf1980 100644 --- a/src/ros2_medkit_gateway/src/ros2/transports/ros2_service_transport.cpp +++ b/src/ros2_medkit_gateway/src/ros2/transports/ros2_service_transport.cpp @@ -24,8 +24,10 @@ namespace ros2_medkit_gateway::ros2 { -Ros2ServiceTransport::Ros2ServiceTransport(rclcpp::Node * node) - : node_(node), serializer_(std::make_shared()) { +Ros2ServiceTransport::Ros2ServiceTransport(rclcpp::Node * node, rclcpp::CallbackGroup::SharedPtr rpc_group) + : node_(node) + , rpc_group_(std::move(rpc_group)) + , serializer_(std::make_shared()) { RCLCPP_INFO(node_->get_logger(), "Ros2ServiceTransport initialised (native serialization)"); } @@ -61,7 +63,7 @@ compat::GenericServiceClient::SharedPtr Ros2ServiceTransport::get_or_create_clie return it->second; } - auto client = compat::create_generic_service_client(node_, service_path, service_type); + auto client = compat::create_generic_service_client(node_, service_path, service_type, rpc_group_); clients_[key] = client; RCLCPP_DEBUG(node_->get_logger(), "Created generic client for %s (%s)", service_path.c_str(), service_type.c_str()); diff --git a/src/ros2_medkit_gateway/src/ros2_common/callback_groups.cpp b/src/ros2_medkit_gateway/src/ros2_common/callback_groups.cpp new file mode 100644 index 000000000..c2f6a12e3 --- /dev/null +++ b/src/ros2_medkit_gateway/src/ros2_common/callback_groups.cpp @@ -0,0 +1,26 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "ros2_medkit_gateway/ros2_common/callback_groups.hpp" + +namespace ros2_medkit_gateway::ros2_common { + +GatewayCallbackGroups create_gateway_callback_groups(rclcpp::Node & node) { + GatewayCallbackGroups groups; + groups.rpc_reentrant = node.create_callback_group(rclcpp::CallbackGroupType::Reentrant); + groups.action_status = node.create_callback_group(rclcpp::CallbackGroupType::MutuallyExclusive); + return groups; +} + +} // namespace ros2_medkit_gateway::ros2_common diff --git a/src/ros2_medkit_gateway/test/test_callback_groups.cpp b/src/ros2_medkit_gateway/test/test_callback_groups.cpp new file mode 100644 index 000000000..8b2455ad0 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_callback_groups.cpp @@ -0,0 +1,102 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Pins the callback-group contract behind server.executor_threads (issue +// #575): the blocking-RPC clients must dispatch on a Reentrant group (so a +// second executor thread can deliver a response while a default-group +// callback runs) and the per-action status subscriptions must dispatch on a +// dedicated MutuallyExclusive group (in-order delivery, decoupled from the +// default group). A regression that silently reverted either group to the +// node default would pass every functional test on an idle system - this +// suite makes the wiring itself falsifiable. + +#include + +#include + +#include +#include + +#include "ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp" +#include "ros2_medkit_gateway/ros2_common/callback_groups.hpp" + +using ros2_medkit_gateway::ros2::Ros2ActionTransport; +using ros2_medkit_gateway::ros2_common::create_gateway_callback_groups; +using ros2_medkit_gateway::ros2_common::GatewayCallbackGroups; + +class CallbackGroupsTest : public ::testing::Test { + protected: + static void SetUpTestSuite() { + rclcpp::init(0, nullptr); + } + + static void TearDownTestSuite() { + if (rclcpp::ok()) { + rclcpp::shutdown(); + } + } + + void SetUp() override { + node_ = std::make_shared("test_callback_groups_node"); + } + + void TearDown() override { + node_.reset(); + } + + std::shared_ptr node_; +}; + +TEST_F(CallbackGroupsTest, RpcGroupIsReentrantStatusGroupIsMutuallyExclusive) { + auto groups = create_gateway_callback_groups(*node_); + + ASSERT_NE(groups.rpc_reentrant, nullptr); + ASSERT_NE(groups.action_status, nullptr); + EXPECT_EQ(groups.rpc_reentrant->type(), rclcpp::CallbackGroupType::Reentrant); + EXPECT_EQ(groups.action_status->type(), rclcpp::CallbackGroupType::MutuallyExclusive); + EXPECT_NE(groups.rpc_reentrant.get(), groups.action_status.get()); +} + +TEST_F(CallbackGroupsTest, GroupsAreDispatchedByTheNodeExecutor) { + // automatically_add_to_executor_with_node makes whichever executor spins + // the node also service these groups - without it, every entity in them + // would silently never fire. + auto groups = create_gateway_callback_groups(*node_); + EXPECT_TRUE(groups.rpc_reentrant->automatically_add_to_executor_with_node()); + EXPECT_TRUE(groups.action_status->automatically_add_to_executor_with_node()); +} + +TEST_F(CallbackGroupsTest, ActionStatusSubscriptionLandsInStatusGroup) { + auto groups = create_gateway_callback_groups(*node_); + Ros2ActionTransport transport(node_.get(), groups.rpc_reentrant, groups.action_status); + + transport.subscribe_status("/powertrain/engine/phantom", + [](const std::string &, const std::string &, ros2_medkit_gateway::ActionGoalStatus) {}); + + // The status subscription must be registered into the shared status group, + // not the node default - otherwise a long default-group callback delays + // goal-status tracking again. Match by topic name: the default group also + // holds rclcpp-internal subscriptions (e.g. /parameter_events), so a bare + // count would not isolate the status subscription. + const std::string status_topic = "/powertrain/engine/phantom/_action/status"; + auto is_status_sub = [&status_topic](const rclcpp::SubscriptionBase::SharedPtr & sub) { + return sub != nullptr && status_topic == sub->get_topic_name(); + }; + + EXPECT_NE(groups.action_status->find_subscription_ptrs_if(is_status_sub), nullptr) + << "status subscription not registered into the shared status group"; + EXPECT_EQ(node_->get_node_base_interface()->get_default_callback_group()->find_subscription_ptrs_if(is_status_sub), + nullptr) + << "status subscription must not land in the node default group"; +} diff --git a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp new file mode 100644 index 000000000..f6cfa4323 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp @@ -0,0 +1,755 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Falsifiers for the cancel-outcome mapping (issue #576). +// +// A real rclcpp action server answers CancelGoal automatically through rcl +// machinery, so it can never be made to swallow the request deterministically. +// This fixture therefore offers `/_action/cancel_goal` as a RAW +// `action_msgs/srv/CancelGoal` service whose callback either blocks past the +// configured budget (the "response lost" case) or rejects immediately, and +// plants the tracked goal via `inject_tracked_goal_for_testing`. The wire +// contract is pinned with literal status codes and code strings on purpose: +// +// - a timed-out cancel is NOT a rejection: 504 + standard `not-responding`, +// tracked status untouched (the /_action/status stream stays the authority); +// - a timed-out cancel whose status stream already shows CANCELING/CANCELED +// is a cancellation in progress: success (204 / 202 for PUT-stop); +// - a genuine server rejection stays 400 + `x-medkit-ros2-action-rejected`. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp" +#include "ros2_medkit_gateway/gateway_node.hpp" +#include "ros2_medkit_gateway/http/typed_router.hpp" + +using json = nlohmann::json; +using ros2_medkit_gateway::ActionGoalInfo; +using ros2_medkit_gateway::ActionGoalStatus; +using ros2_medkit_gateway::AuthConfig; +using ros2_medkit_gateway::CorsConfig; +using ros2_medkit_gateway::GatewayNode; +using ros2_medkit_gateway::TlsConfig; +using ros2_medkit_gateway::handlers::HandlerContext; +using ros2_medkit_gateway::handlers::OperationHandlers; +namespace dto = ros2_medkit_gateway::dto; +namespace http = ros2_medkit_gateway::http; + +namespace { + +using namespace std::chrono_literals; + +/// Multiplier for the wall-clock budgets this fixture waits on. The sanitizer +/// CI jobs run the unit suite too, and an ASan/TSan-instrumented GatewayNode +/// takes materially longer to bring a service up - a budget that is generous +/// unsanitized can be tight under instrumentation, and the failure then reads +/// as a #576 regression rather than as overhead. The jobs export +/// MEDKIT_TEST_TIME_SCALE with the same factor they apply to every ctest +/// TIMEOUT; unset / unparseable / below 1 means no scaling, so the normal +/// jobs keep the tight budgets. +double test_time_scale() { + const char * raw = std::getenv("MEDKIT_TEST_TIME_SCALE"); + if (raw == nullptr) { + return 1.0; + } + try { + const double scale = std::stod(raw); + return scale >= 1.0 ? scale : 1.0; + } catch (const std::exception &) { + return 1.0; + } +} + +/// Scale a wall-clock budget by `test_time_scale()`. +std::chrono::seconds scaled(std::chrono::seconds base) { + return std::chrono::seconds{static_cast(static_cast(base.count()) * test_time_scale())}; +} + +int reserve_local_port() { + int sock = socket(AF_INET, SOCK_STREAM, 0); + if (sock < 0) { + ADD_FAILURE() << "Failed to create socket for test port reservation: " << std::strerror(errno); + return 0; + } + + sockaddr_in addr{}; + addr.sin_family = AF_INET; + addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + addr.sin_port = 0; + + if (bind(sock, reinterpret_cast(&addr), sizeof(addr)) != 0) { + ADD_FAILURE() << "Failed to bind socket for test port reservation: " << std::strerror(errno); + close(sock); + return 0; + } + + socklen_t addr_len = sizeof(addr); + if (getsockname(sock, reinterpret_cast(&addr), &addr_len) != 0) { + ADD_FAILURE() << "Failed to inspect reserved test port: " << std::strerror(errno); + close(sock); + return 0; + } + + int port = ntohs(addr.sin_port); + close(sock); + return port; +} + +httplib::Request make_request_with_match(const std::string & path, const std::string & pattern) { + httplib::Request req; + req.path = path; + std::regex re(pattern); + std::regex_match(req.path, req.matches, re); + return req; +} + +/// Raw CancelGoal service + status publisher standing in for an action +/// server whose cancel path misbehaves. Three modes: +/// - kBlockUntilReleased: the service callback parks on a condition variable +/// (bounded, releasable) so the caller's response future must time out - +/// the deterministic "cancel response lost" case a real action server +/// cannot produce. +/// - kRejectImmediately: replies with `reject_return_code_` (1/2/3) at once - +/// the definitive-rejection cases. +/// - kAcceptImmediately: replies ERROR_NONE at once - the accepted cancel, +/// used to race a stream-delivered terminal status against the kOk write. +class PhantomCancelFixtureNode : public rclcpp::Node { + public: + enum class CancelMode { kBlockUntilReleased, kRejectImmediately, kAcceptImmediately }; + + PhantomCancelFixtureNode() : rclcpp::Node("phantom_cancel_fixture", "/powertrain/engine") { + cancel_service_ = create_service( + "phantom_calibration/_action/cancel_goal", + [this](const std::shared_ptr & /*request*/, + const std::shared_ptr & response) { + if (mode_.load() == CancelMode::kRejectImmediately) { + response->return_code = reject_return_code_.load(); + return; + } + if (mode_.load() == CancelMode::kAcceptImmediately) { + response->return_code = action_msgs::srv::CancelGoal::Response::ERROR_NONE; + return; + } + // Swallow the request past any realistic budget so the caller's + // future times out. Bounded and releasable so teardown never hangs + // an executor thread. + std::unique_lock lock(release_mutex_); + release_cv_.wait_for(lock, scaled(std::chrono::seconds(30)), [this] { + return released_; + }); + response->return_code = action_msgs::srv::CancelGoal::Response::ERROR_NONE; + }); + // Publish with the profile a real action server offers - + // rcl_action_qos_profile_status_default: KEEP_LAST(1), RELIABLE, + // TRANSIENT_LOCAL. This stands in for an action server's status + // publisher, so it has to make the same offer: a VOLATILE writer is + // durability-incompatible with the gateway's TRANSIENT_LOCAL reader and + // would never match it, which no real deployment can reproduce. + status_pub_ = create_publisher( + "phantom_calibration/_action/status", rclcpp::QoS(rclcpp::KeepLast(1)).reliable().transient_local()); + } + + // Subscription-destructor pattern: the service callback captures `this`, + // so it must be released and dropped before member destruction begins. + ~PhantomCancelFixtureNode() override { + release_blocked_cancels(); + cancel_service_.reset(); + status_pub_.reset(); + } + + PhantomCancelFixtureNode(const PhantomCancelFixtureNode &) = delete; + PhantomCancelFixtureNode & operator=(const PhantomCancelFixtureNode &) = delete; + PhantomCancelFixtureNode(PhantomCancelFixtureNode &&) = delete; + PhantomCancelFixtureNode & operator=(PhantomCancelFixtureNode &&) = delete; + + void set_mode(CancelMode mode) { + mode_.store(mode); + } + + /// Return code replied in kRejectImmediately mode. Defaults to + /// ERROR_REJECTED (1); set to 2 / 3 to drive the other documented + /// rejection codes. + void set_reject_return_code(int8_t code) { + reject_return_code_.store(code); + } + + void release_blocked_cancels() { + { + std::lock_guard lock(release_mutex_); + released_ = true; + } + release_cv_.notify_all(); + } + + void publish_status(const std::array & uuid, int8_t status_byte) { + action_msgs::msg::GoalStatusArray msg; + action_msgs::msg::GoalStatus status; + status.goal_info.goal_id.uuid = uuid; + status.status = status_byte; + msg.status_list.push_back(status); + status_pub_->publish(msg); + } + + private: + rclcpp::Service::SharedPtr cancel_service_; + rclcpp::Publisher::SharedPtr status_pub_; + std::atomic mode_{CancelMode::kBlockUntilReleased}; + std::atomic reject_return_code_{action_msgs::srv::CancelGoal::Response::ERROR_REJECTED}; + std::mutex release_mutex_; + std::condition_variable release_cv_; + bool released_{false}; +}; + +} // namespace + +class CancelOutcomesFixtureTest : public ::testing::Test { + protected: + static constexpr const char * kActionPath = "/powertrain/engine/phantom_calibration"; + static constexpr const char * kGoalIdHex = "00112233445566778899aabbccddeeff"; + static constexpr const char * kCancelServiceName = "/powertrain/engine/phantom_calibration/_action/cancel_goal"; + + static inline int suite_server_port_ = 0; + + static void SetUpTestSuite() { + suite_server_port_ = reserve_local_port(); + ASSERT_NE(suite_server_port_, 0); + + // service_call_timeout_sec:=1 keeps the blocking-cancel tests fast: the + // cancel budget must follow the configured timeout (no hidden floor). + // Refresh + debounce are pinned high so discovery churn cannot interfere. + std::vector args = {"test_cancel_outcomes", + "--ros-args", + "-p", + "server.port:=" + std::to_string(suite_server_port_), + "-p", + "refresh_interval_ms:=60000", + "-p", + "discovery.refresh_debounce_ms:=60000", + "-p", + "service_call_timeout_sec:=1"}; + + std::vector argv; + argv.reserve(args.size()); + for (auto & arg : args) { + argv.push_back(arg.data()); + } + + rclcpp::init(static_cast(argv.size()), argv.data()); + } + + static void TearDownTestSuite() { + if (rclcpp::ok()) { + rclcpp::shutdown(); + } + } + + void SetUp() override { + gateway_node_ = std::make_shared(); + ASSERT_NE(gateway_node_, nullptr); + fixture_node_ = std::make_shared(); + + executor_ = std::make_unique(); + executor_->add_node(gateway_node_); + executor_->add_node(fixture_node_); + spin_thread_ = std::thread([this]() { + executor_->spin(); + }); + + ctx_ = std::make_unique(gateway_node_.get(), cors_, auth_, tls_, nullptr); + handlers_ = std::make_unique(*ctx_); + + // The transport's wait_for_service must resolve the phantom cancel + // service, otherwise every test would exercise the unavailability path + // instead of its intended outcome. + ASSERT_TRUE(wait_for_cancel_service()) << "phantom cancel service not discovered by the gateway participant"; + + prime_action_clients(); + } + + void TearDown() override { + // Release any parked cancel callback BEFORE cancelling the executor: + // a thread stuck inside the blocking service callback would otherwise + // hold up spin_thread_.join() for the fixture's 30s backstop. + if (fixture_node_ != nullptr) { + fixture_node_->release_blocked_cancels(); + } + if (executor_ != nullptr) { + executor_->cancel(); + } + if (spin_thread_.joinable()) { + spin_thread_.join(); + } + + handlers_.reset(); + ctx_.reset(); + executor_.reset(); + fixture_node_.reset(); + gateway_node_.reset(); + } + + /// Cache the transport's client trio for the phantom action with its real + /// interface type. In production, cancel always follows a send through this + /// gateway, so the trio is already cached with the correct action type; + /// injected goals bypass send, and a cancel without cached clients would + /// take the placeholder-type path instead of the outcome under test. The + /// send itself fails fast (no send_goal server exists) - only its + /// client-creation side effect matters. + void prime_action_clients() { + auto sent = gateway_node_->get_operation_manager()->send_action_goal( + kActionPath, "example_interfaces/action/Fibonacci", json::object(), "engine"); + ASSERT_FALSE(sent.success) << "no send_goal server exists - the priming send must fail"; + } + + bool wait_for_cancel_service(std::chrono::seconds timeout = scaled(std::chrono::seconds(15))) { + const auto deadline = std::chrono::steady_clock::now() + timeout; + while (std::chrono::steady_clock::now() < deadline) { + const auto services = gateway_node_->get_service_names_and_types(); + if (services.count(kCancelServiceName) > 0) { + return true; + } + std::this_thread::sleep_for(std::chrono::milliseconds(20)); + } + return false; + } + + void inject_goal(ActionGoalStatus status = ActionGoalStatus::EXECUTING) { + ActionGoalInfo info; + info.goal_id = kGoalIdHex; + info.action_path = kActionPath; + info.action_type = "example_interfaces/action/Fibonacci"; + info.entity_id = "engine"; + info.status = status; + info.created_at = std::chrono::system_clock::now(); + info.last_update = info.created_at; + gateway_node_->get_operation_manager()->inject_tracked_goal_for_testing(std::move(info)); + } + + static std::array goal_id_bytes() { + std::array bytes{}; + const std::string hex = kGoalIdHex; + for (size_t i = 0; i < bytes.size(); ++i) { + bytes[i] = static_cast(std::stoi(hex.substr(i * 2, 2), nullptr, 16)); + } + return bytes; + } + + /// Subscribe the manager to the phantom action's status stream (the seam + /// `send_action_goal` uses in production - public on OperationManager) and + /// publish raw GoalStatusArray frames until the tracked goal reflects + /// `wanted`. Publishing repeats because the best-effort subscription may + /// not have matched yet on the first frame. + bool deliver_status_until_tracked(ActionGoalStatus wanted, int8_t status_byte) { + auto * operation_mgr = gateway_node_->get_operation_manager(); + operation_mgr->subscribe_to_action_status(kActionPath); + const auto uuid = goal_id_bytes(); + const auto deadline = std::chrono::steady_clock::now() + scaled(std::chrono::seconds(10)); + while (std::chrono::steady_clock::now() < deadline) { + fixture_node_->publish_status(uuid, status_byte); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + auto tracked = operation_mgr->get_tracked_goal(kGoalIdHex); + if (tracked.has_value() && tracked->status == wanted) { + return true; + } + } + return false; + } + + http::TypedRequest make_execution_request() { + return make_execution_request_for("components"); + } + + /// The DELETE/PUT execution routes are registered for every entity + /// collection (apps, components, areas, functions), so the fixture has to + /// be able to drive any of them - a response that hardcodes one collection + /// is only observable from another. + http::TypedRequest make_execution_request_for(const std::string & collection) { + raw_req_ = make_request_with_match("/api/v1/" + collection + "/engine/operations/phantom_calibration/executions/" + + kGoalIdHex, + "/api/v1/" + collection + R"(/([^/]+)/operations/([^/]+)/executions/([^/]+))"); + return http::TypedRequest(raw_req_); + } + + ActionGoalStatus tracked_status_or_fail() { + auto tracked = gateway_node_->get_operation_manager()->get_tracked_goal(kGoalIdHex); + EXPECT_TRUE(tracked.has_value()); + return tracked.has_value() ? tracked->status : ActionGoalStatus::UNKNOWN; + } + + CorsConfig cors_{}; + AuthConfig auth_{}; + TlsConfig tls_{}; + httplib::Request raw_req_; + std::shared_ptr gateway_node_; + std::shared_ptr fixture_node_; + std::unique_ptr executor_; + std::thread spin_thread_; + std::unique_ptr ctx_; + std::unique_ptr handlers_; +}; + +// --------------------------------------------------------------------------- +// DELETE /{entity}/operations/{op}/executions/{id} +// --------------------------------------------------------------------------- + +TEST_F(CancelOutcomesFixtureTest, CancelTimeoutReturns504NotRespondingAndLeavesTrackedStatus) { + inject_goal(); + auto typed = make_execution_request(); + + auto result = handlers_->cancel_execution(typed); + + ASSERT_FALSE(result.has_value()) << "a swallowed cancel must not be reported as success"; + EXPECT_EQ(result.error().http_status, 504) << result.error().code << ": " << result.error().message; + EXPECT_EQ(result.error().code, "not-responding"); + // The advice must name a field that can actually express the answer: the + // SOVD `status` renders CANCELED and ABORTED identically as "failed", so + // polling it alone can never tell a client whether its cancel took effect. + EXPECT_NE(result.error().message.find("x-medkit.ros2_status"), std::string::npos) + << "the message points at a resource but not at the field that carries the outcome: " << result.error().message; + // The outcome is unknown - the handler must not fabricate a tracked status; + // the /_action/status stream stays the authority. + EXPECT_EQ(tracked_status_or_fail(), ActionGoalStatus::EXECUTING); +} + +TEST_F(CancelOutcomesFixtureTest, CancelTimeoutWithCancelingStatusReturns204) { + inject_goal(); + ASSERT_TRUE(deliver_status_until_tracked(ActionGoalStatus::CANCELING, action_msgs::msg::GoalStatus::STATUS_CANCELING)) + << "status stream never reached the tracked goal"; + + auto typed = make_execution_request(); + auto result = handlers_->cancel_execution(typed); + + ASSERT_TRUE(result.has_value()) << "cancel must reconcile against the status stream: " << result.error().code << ": " + << result.error().message; + EXPECT_EQ(tracked_status_or_fail(), ActionGoalStatus::CANCELING); + + // GET execution status must agree with what the cancel response implied. + auto get_typed = make_execution_request(); + auto exec = handlers_->get_execution(get_typed); + ASSERT_TRUE(exec.has_value()); + EXPECT_EQ(exec->status, "running"); // CANCELING renders as SOVD "running" + ASSERT_TRUE(exec->x_medkit.has_value()); + ASSERT_TRUE(exec->x_medkit->ros2_status.has_value()); + EXPECT_EQ(*exec->x_medkit->ros2_status, "canceling"); +} + +TEST_F(CancelOutcomesFixtureTest, CancelRejectedByServerReturns400Rejected) { + fixture_node_->set_mode(PhantomCancelFixtureNode::CancelMode::kRejectImmediately); + inject_goal(); + auto typed = make_execution_request(); + + auto result = handlers_->cancel_execution(typed); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 400); + EXPECT_EQ(result.error().code, "x-medkit-ros2-action-rejected"); + EXPECT_EQ(result.error().message, "Cancel request rejected"); + EXPECT_EQ(result.error().params["return_code"], 1); +} + +// --------------------------------------------------------------------------- +// PUT /{entity}/operations/{op}/executions/{id} with {"capability": "stop"} +// --------------------------------------------------------------------------- + +TEST_F(CancelOutcomesFixtureTest, PutStopTimeoutReturns504NotResponding) { + inject_goal(); + auto typed = make_execution_request(); + dto::ExecutionUpdateRequest body; + body.capability = "stop"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_FALSE(result.has_value()) << "a swallowed stop must not be reported as accepted"; + EXPECT_EQ(result.error().http_status, 504) << result.error().code << ": " << result.error().message; + EXPECT_EQ(result.error().code, "not-responding"); + EXPECT_EQ(tracked_status_or_fail(), ActionGoalStatus::EXECUTING); +} + +TEST_F(CancelOutcomesFixtureTest, PutStopTimeoutWithCancelingStatusReturns202) { + inject_goal(); + ASSERT_TRUE(deliver_status_until_tracked(ActionGoalStatus::CANCELING, action_msgs::msg::GoalStatus::STATUS_CANCELING)) + << "status stream never reached the tracked goal"; + + auto typed = make_execution_request(); + dto::ExecutionUpdateRequest body; + body.capability = "stop"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_TRUE(result.has_value()) << "stop must reconcile against the status stream: " << result.error().code << ": " + << result.error().message; + const auto & att = result.value().second; + ASSERT_TRUE(att.status_override.has_value()); + EXPECT_EQ(*att.status_override, 202); + EXPECT_EQ(result.value().first.status, "running"); + EXPECT_EQ(tracked_status_or_fail(), ActionGoalStatus::CANCELING); +} + +TEST_F(CancelOutcomesFixtureTest, PutStopRejectedByServerReturns400Rejected) { + fixture_node_->set_mode(PhantomCancelFixtureNode::CancelMode::kRejectImmediately); + inject_goal(); + auto typed = make_execution_request(); + dto::ExecutionUpdateRequest body; + body.capability = "stop"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 400); + EXPECT_EQ(result.error().code, "x-medkit-ros2-action-rejected"); + EXPECT_EQ(result.error().message, "Stop request rejected"); +} + +// --------------------------------------------------------------------------- +// The accepted-cancel write must not move a terminal tracked status backwards +// --------------------------------------------------------------------------- + +TEST_F(CancelOutcomesFixtureTest, AcceptedCancelKeepsStreamDeliveredTerminalStatus) { + // The status stream and the CancelGoal response race: an action server that + // cancels immediately publishes STATUS_CANCELED before its rc=0 answer is + // processed. The branch's own contract is that the stream stays the + // authority - hand-writing CANCELING on top of a terminal CANCELED is a + // backwards transition that nothing ever corrects (no further status frame + // is published), leaving GET reporting "running" forever and the goal only + // force-evicted through the stuck-goal path with a false "server crashed" + // warning. + fixture_node_->set_mode(PhantomCancelFixtureNode::CancelMode::kAcceptImmediately); + inject_goal(); + ASSERT_TRUE(deliver_status_until_tracked(ActionGoalStatus::CANCELED, action_msgs::msg::GoalStatus::STATUS_CANCELED)) + << "status stream never reached the tracked goal"; + + auto typed = make_execution_request(); + auto result = handlers_->cancel_execution(typed); + + ASSERT_TRUE(result.has_value()) << "an accepted cancel is still a success: " << result.error().code << ": " + << result.error().message; + EXPECT_EQ(tracked_status_or_fail(), ActionGoalStatus::CANCELED) + << "the accepted-cancel write downgraded a terminal status delivered by the stream"; +} + +// --------------------------------------------------------------------------- +// The reconciled 202 body must agree with the resource it points at +// --------------------------------------------------------------------------- + +TEST_F(CancelOutcomesFixtureTest, PutStopReconciledBodyStatusAgreesWithGetExecution) { + // Reconcile set includes CANCELED, which GET renders as "failed"; a 202 + // body hardcoding "running" contradicts the very resource its Location + // header points at. + inject_goal(); + ASSERT_TRUE(deliver_status_until_tracked(ActionGoalStatus::CANCELED, action_msgs::msg::GoalStatus::STATUS_CANCELED)) + << "status stream never reached the tracked goal"; + + auto typed = make_execution_request(); + dto::ExecutionUpdateRequest body; + body.capability = "stop"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_TRUE(result.has_value()) << "stop must reconcile against the status stream: " << result.error().code << ": " + << result.error().message; + ASSERT_TRUE(result.value().second.status_override.has_value()); + EXPECT_EQ(*result.value().second.status_override, 202); + + auto get_typed = make_execution_request(); + auto exec = handlers_->get_execution(get_typed); + ASSERT_TRUE(exec.has_value()); + EXPECT_EQ(result.value().first.status, exec->status) + << "the 202 body contradicts an immediate GET of the same execution"; +} + +TEST_F(CancelOutcomesFixtureTest, PutStopReconciledLocationPointsAtTheRequestedCollection) { + // The same route is registered for apps, components, areas and functions. + // A Location built from a hardcoded apps/components pair sends an areas + // client into the components collection. + inject_goal(); + ASSERT_TRUE(deliver_status_until_tracked(ActionGoalStatus::CANCELING, action_msgs::msg::GoalStatus::STATUS_CANCELING)) + << "status stream never reached the tracked goal"; + + auto typed = make_execution_request_for("areas"); + const std::string requested_path = std::string(typed.path()); + dto::ExecutionUpdateRequest body; + body.capability = "stop"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_TRUE(result.has_value()) << result.error().code << ": " << result.error().message; + const auto & headers = result.value().second.headers; + auto location = std::find_if(headers.begin(), headers.end(), [](const auto & kv) { + return kv.first == "Location"; + }); + ASSERT_NE(location, headers.end()) << "202 must carry a Location header"; + EXPECT_EQ(location->second, requested_path); +} + +// --------------------------------------------------------------------------- +// Terminal tracked status on the timeout path +// --------------------------------------------------------------------------- + +TEST_F(CancelOutcomesFixtureTest, CancelTimeoutWithTerminalStatusReturns504WithoutPromisingProgress) { + // The cancel RPC really did go unanswered (504 stands, ruling R5), but the + // gateway's own tracked state proves the goal is terminal: telling the + // client to poll for "the goal's progress" describes something that cannot + // happen. + inject_goal(ActionGoalStatus::SUCCEEDED); + auto typed = make_execution_request(); + + auto result = handlers_->cancel_execution(typed); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 504) << result.error().code << ": " << result.error().message; + EXPECT_EQ(result.error().code, "not-responding"); + EXPECT_EQ(result.error().message.find("progress"), std::string::npos) + << "a terminal goal has no progress to observe: " << result.error().message; + + // Pin the AGREEMENT, not the string the handler happens to print: the + // message names the execution status resource, so the word it quotes has to + // be the word that resource answers with. Asserting a literal here would + // stay green if the message drifted away from the endpoint it cites. + auto get_typed = make_execution_request(); + auto exec = handlers_->get_execution(get_typed); + ASSERT_TRUE(exec.has_value()); + EXPECT_NE(result.error().message.find(exec->status), std::string::npos) + << "the 504 message must quote the execution resource's own vocabulary (" << exec->status + << "): " << result.error().message; + EXPECT_EQ(tracked_status_or_fail(), ActionGoalStatus::SUCCEEDED); +} + +TEST_F(CancelOutcomesFixtureTest, TimeoutErrorParametersCarryNoReturnCode) { + // `return_code: 0` on a path where no server return code exists is exactly + // the ambiguity issue #576 is about. + inject_goal(); + auto typed = make_execution_request(); + + auto result = handlers_->cancel_execution(typed); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 504); + EXPECT_FALSE(result.error().params.contains("return_code")) + << "no server return code exists on the timeout path: " << result.error().params.dump(); +} + +// The 409 on PUT-execute must carry a code from the SOVD standard list. +// `invalid-request` is not in it (knowledge/technology/sovd/general_aspects.rst, +// "SOVD Error Codes"); `precondition-not-fulfilled` ("Prerequisites not met") +// is, and the gateway already answers its other 409 with it +// (lifecycle_handlers.cpp). One status, one code, across the whole server. +TEST_F(CancelOutcomesFixtureTest, ReExecuteOnARunningExecutionCarriesTheStandard409Code) { + inject_goal(); + auto typed = make_execution_request(); + dto::ExecutionUpdateRequest body; + body.capability = "execute"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 409); + EXPECT_EQ(result.error().code, "precondition-not-fulfilled") << result.error().message; +} + +// --------------------------------------------------------------------------- +// Rejection codes 2 and 3 at both entry points +// --------------------------------------------------------------------------- + +// The docs promise 400 for return_code 1-3 at both entry points, but only +// rc=1 was ever driven - a mapper case for rc=2 or rc=3 could be deleted and +// the suite would stay green while the wire message silently changed to the +// transport's differently-worded copy. +TEST_F(CancelOutcomesFixtureTest, CancelRejectionCodesEachMapTo400WithTheirOwnMessage) { + fixture_node_->set_mode(PhantomCancelFixtureNode::CancelMode::kRejectImmediately); + inject_goal(); + + const std::pair cases[] = { + {1, "Cancel request rejected"}, {2, "Unknown execution ID"}, {3, "Execution already terminated"}}; + for (const auto & [code, expected_message] : cases) { + SCOPED_TRACE("return_code=" + std::to_string(static_cast(code))); + fixture_node_->set_reject_return_code(code); + auto typed = make_execution_request(); + + auto result = handlers_->cancel_execution(typed); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 400); + EXPECT_EQ(result.error().code, "x-medkit-ros2-action-rejected"); + EXPECT_EQ(result.error().message, expected_message); + EXPECT_EQ(result.error().params["return_code"], code); + } +} + +TEST_F(CancelOutcomesFixtureTest, PutStopRejectionCodesEachMapTo400WithTheirOwnMessage) { + fixture_node_->set_mode(PhantomCancelFixtureNode::CancelMode::kRejectImmediately); + inject_goal(); + + const std::pair cases[] = { + {1, "Stop request rejected"}, {2, "Unknown execution ID"}, {3, "Execution already terminated"}}; + for (const auto & [code, expected_message] : cases) { + SCOPED_TRACE("return_code=" + std::to_string(static_cast(code))); + fixture_node_->set_reject_return_code(code); + auto typed = make_execution_request(); + dto::ExecutionUpdateRequest body; + body.capability = "stop"; + + auto result = handlers_->update_execution(typed, body); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 400); + EXPECT_EQ(result.error().code, "x-medkit-ros2-action-rejected"); + EXPECT_EQ(result.error().message, expected_message); + EXPECT_EQ(result.error().params["return_code"], code); + } +} + +// --------------------------------------------------------------------------- +// Manager guard exits reaching the wire +// --------------------------------------------------------------------------- + +TEST_F(CancelOutcomesFixtureTest, ExecutionEvictedBetweenChecksMapsTo404NotFound) { + // The handler validates the execution exists, then the cleanup timer can + // evict it before the manager's own re-check. The truthful answer is + // "execution no longer exists" (404) - the same answer the request gets a + // millisecond later - not "action server unavailable" (500). + auto * operation_mgr = gateway_node_->get_operation_manager(); + ASSERT_FALSE(operation_mgr->get_tracked_goal(kGoalIdHex).has_value()); + + auto result = operation_mgr->cancel_action_goal(kActionPath, kGoalIdHex); + auto failure = ros2_medkit_gateway::handlers::detail::map_cancel_result(result, *operation_mgr, kGoalIdHex, "Cancel"); + + ASSERT_TRUE(failure.has_value()); + EXPECT_EQ(failure->http_status, 404) << failure->error_code << ": " << failure->message; + EXPECT_STREQ(failure->error_code, "resource-not-found"); +} diff --git a/src/ros2_medkit_gateway/test/test_gateway_node.cpp b/src/ros2_medkit_gateway/test/test_gateway_node.cpp index d0451a58a..2f81dbc6c 100644 --- a/src/ros2_medkit_gateway/test/test_gateway_node.cpp +++ b/src/ros2_medkit_gateway/test/test_gateway_node.cpp @@ -32,6 +32,7 @@ #include "ros2_medkit_gateway/core/discovery/models/function.hpp" #include "ros2_medkit_gateway/core/http/http_utils.hpp" +#include "ros2_medkit_gateway/core/managers/operation_manager.hpp" #include "ros2_medkit_gateway/fault_manager_paths.hpp" #include "ros2_medkit_gateway/gateway_node.hpp" @@ -278,6 +279,40 @@ TEST_F(TestGatewayNode, test_fault_manager_namespace_configures_event_subscriber EXPECT_EQ(res->status, 200); } +// `service_call_timeout_sec` is the whole cancel budget since the 15s floor +// was removed (issue #576), yet it was declared with no bounds at all: 0 and +// negatives were accepted silently (a cancel wait of 0 ms) and an absurd +// value would pin an HTTP worker for as long as it says. Sweep the documented +// range's endpoints and both degenerate directions, and observe the budget +// the gateway actually applies rather than the value that was configured. +TEST_F(TestGatewayNode, test_service_call_timeout_sec_is_clamped_to_the_documented_range) { + const std::pair cases[] = { + {0, 1}, // degenerate: no budget at all -> floor + {-5, 1}, // negative -> floor + {1, 1}, // range floor, accepted as-is + {10, 10}, // default, accepted as-is + {3600, 3600}, // range ceiling, accepted as-is + {100000, 3600} + // above the ceiling -> ceiling + }; + + for (const auto & [configured, expected] : cases) { + SCOPED_TRACE("service_call_timeout_sec=" + std::to_string(configured)); + node_.reset(); + + int free_port = reserve_free_port(); + ASSERT_NE(free_port, 0) << "Failed to reserve a free port for test"; + create_node_with_overrides({ + rclcpp::Parameter("server.port", free_port), + rclcpp::Parameter("service_call_timeout_sec", configured), + }); + + auto * operation_mgr = node_->get_operation_manager(); + ASSERT_NE(operation_mgr, nullptr); + EXPECT_EQ(operation_mgr->service_call_timeout_sec(), expected); + } +} + TEST_F(TestGatewayNode, test_version_info_endpoint) { // @verifies REQ_INTEROP_001 auto client = create_client(); diff --git a/src/ros2_medkit_gateway/test/test_generic_client_compat.cpp b/src/ros2_medkit_gateway/test/test_generic_client_compat.cpp index 29a7a4e87..868576881 100644 --- a/src/ros2_medkit_gateway/test/test_generic_client_compat.cpp +++ b/src/ros2_medkit_gateway/test/test_generic_client_compat.cpp @@ -47,34 +47,44 @@ class TestGenericClientCompat : public ::testing::Test { void SetUp() override { node_ = std::make_shared("test_generic_client_compat_node"); + // Register clients into a Reentrant group, matching production wiring + // (issue #575): both compat paths must honour the group argument. + rpc_group_ = node_->create_callback_group(rclcpp::CallbackGroupType::Reentrant); } void TearDown() override { + rpc_group_.reset(); node_.reset(); } std::shared_ptr node_; + rclcpp::CallbackGroup::SharedPtr rpc_group_; }; // ==================== FACTORY TESTS ==================== /// Factory creates a valid non-null client TEST_F(TestGenericClientCompat, factory_creates_valid_client) { - auto client = compat::create_generic_service_client(node_.get(), "/test/trigger_service", "std_srvs/srv/Trigger"); + auto client = + compat::create_generic_service_client(node_.get(), "/test/trigger_service", "std_srvs/srv/Trigger", rpc_group_); ASSERT_NE(client, nullptr); } /// Factory works with different service types TEST_F(TestGenericClientCompat, factory_works_with_set_bool) { - auto client = compat::create_generic_service_client(node_.get(), "/test/set_bool_service", "std_srvs/srv/SetBool"); + auto client = + compat::create_generic_service_client(node_.get(), "/test/set_bool_service", "std_srvs/srv/SetBool", rpc_group_); ASSERT_NE(client, nullptr); } /// Multiple clients can be created for different services TEST_F(TestGenericClientCompat, multiple_clients_for_different_services) { - auto client_a = compat::create_generic_service_client(node_.get(), "/test/service_a", "std_srvs/srv/Trigger"); - auto client_b = compat::create_generic_service_client(node_.get(), "/test/service_b", "std_srvs/srv/Trigger"); - auto client_c = compat::create_generic_service_client(node_.get(), "/test/service_c", "std_srvs/srv/SetBool"); + auto client_a = + compat::create_generic_service_client(node_.get(), "/test/service_a", "std_srvs/srv/Trigger", rpc_group_); + auto client_b = + compat::create_generic_service_client(node_.get(), "/test/service_b", "std_srvs/srv/Trigger", rpc_group_); + auto client_c = + compat::create_generic_service_client(node_.get(), "/test/service_c", "std_srvs/srv/SetBool", rpc_group_); ASSERT_NE(client_a, nullptr); ASSERT_NE(client_b, nullptr); @@ -87,8 +97,10 @@ TEST_F(TestGenericClientCompat, multiple_clients_for_different_services) { /// Multiple clients can be created for the same service (different consumers) TEST_F(TestGenericClientCompat, multiple_clients_for_same_service) { - auto client1 = compat::create_generic_service_client(node_.get(), "/test/shared_service", "std_srvs/srv/Trigger"); - auto client2 = compat::create_generic_service_client(node_.get(), "/test/shared_service", "std_srvs/srv/Trigger"); + auto client1 = + compat::create_generic_service_client(node_.get(), "/test/shared_service", "std_srvs/srv/Trigger", rpc_group_); + auto client2 = + compat::create_generic_service_client(node_.get(), "/test/shared_service", "std_srvs/srv/Trigger", rpc_group_); ASSERT_NE(client1, nullptr); ASSERT_NE(client2, nullptr); @@ -99,8 +111,8 @@ TEST_F(TestGenericClientCompat, multiple_clients_for_same_service) { /// Client reports service as unavailable when no server exists TEST_F(TestGenericClientCompat, service_not_available_for_nonexistent) { - auto client = - compat::create_generic_service_client(node_.get(), "/nonexistent/trigger_service", "std_srvs/srv/Trigger"); + auto client = compat::create_generic_service_client(node_.get(), "/nonexistent/trigger_service", + "std_srvs/srv/Trigger", rpc_group_); ASSERT_NE(client, nullptr); bool available = client->wait_for_service(std::chrono::milliseconds(100)); @@ -117,8 +129,8 @@ TEST_F(TestGenericClientCompat, detects_running_service) { res->message = "ok"; }); - auto client = - compat::create_generic_service_client(node_.get(), "/test/live_trigger_service", "std_srvs/srv/Trigger"); + auto client = compat::create_generic_service_client(node_.get(), "/test/live_trigger_service", "std_srvs/srv/Trigger", + rpc_group_); ASSERT_NE(client, nullptr); // Service should become available @@ -135,8 +147,8 @@ TEST_F(TestGenericClientCompat, detects_running_set_bool_service) { res->message = "done"; }); - auto client = - compat::create_generic_service_client(node_.get(), "/test/live_set_bool_service", "std_srvs/srv/SetBool"); + auto client = compat::create_generic_service_client(node_.get(), "/test/live_set_bool_service", + "std_srvs/srv/SetBool", rpc_group_); ASSERT_NE(client, nullptr); bool available = client->wait_for_service(std::chrono::seconds(2)); @@ -147,8 +159,8 @@ TEST_F(TestGenericClientCompat, detects_running_set_bool_service) { /// GenericServiceClient::SharedPtr is a valid shared_ptr type TEST_F(TestGenericClientCompat, shared_ptr_type_is_valid) { - compat::GenericServiceClient::SharedPtr client = - compat::create_generic_service_client(node_.get(), "/test/type_alias_service", "std_srvs/srv/Trigger"); + compat::GenericServiceClient::SharedPtr client = compat::create_generic_service_client( + node_.get(), "/test/type_alias_service", "std_srvs/srv/Trigger", rpc_group_); // Verify it's a proper shared_ptr (use_count should be >= 1) ASSERT_NE(client, nullptr); @@ -157,7 +169,8 @@ TEST_F(TestGenericClientCompat, shared_ptr_type_is_valid) { /// Client can be stored as a ClientBase shared pointer (polymorphism) TEST_F(TestGenericClientCompat, can_cast_to_client_base) { - auto client = compat::create_generic_service_client(node_.get(), "/test/cast_service", "std_srvs/srv/Trigger"); + auto client = + compat::create_generic_service_client(node_.get(), "/test/cast_service", "std_srvs/srv/Trigger", rpc_group_); ASSERT_NE(client, nullptr); // The compat client (on both paths) should be castable to ClientBase diff --git a/src/ros2_medkit_gateway/test/test_operation_handlers.cpp b/src/ros2_medkit_gateway/test/test_operation_handlers.cpp index 91d5241a3..27590fc7f 100644 --- a/src/ros2_medkit_gateway/test/test_operation_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_handlers.cpp @@ -25,6 +25,7 @@ #include #include +#include #include #include #include @@ -38,6 +39,7 @@ #include #include +#include "ros2_medkit_gateway/core/discovery/models/area.hpp" #include "ros2_medkit_gateway/core/http/error_codes.hpp" #include "ros2_medkit_gateway/core/http/handlers/operation_handlers.hpp" #include "ros2_medkit_gateway/dto/json_writer.hpp" @@ -48,6 +50,7 @@ using json = nlohmann::json; using ros2_medkit_gateway::ActionGoalInfo; using ros2_medkit_gateway::ActionGoalStatus; using ros2_medkit_gateway::ActionInfo; +using ros2_medkit_gateway::Area; using ros2_medkit_gateway::AuthConfig; using ros2_medkit_gateway::Component; using ros2_medkit_gateway::CorsConfig; @@ -382,8 +385,18 @@ class OperationHandlersFixtureTest : public ::testing::Test { component.actions = {ActionInfo{"long_calibration", "/powertrain/engine/long_calibration", "example_interfaces/action/Fibonacci", std::nullopt}}; + // The area the component sits in. The operation routes are registered for + // all four entity types and create_execution rejects a collection / + // entity-type mismatch, so exercising a non-component collection needs a + // real entity of that type; an area aggregates its components' operations. + Area area; + area.id = "powertrain"; + area.name = "Powertrain"; + area.namespace_path = "/powertrain"; + area.source = "manifest"; + auto & cache = const_cast(gateway_node_->get_thread_safe_cache()); - cache.update_all({}, {component}, {}, {}); + cache.update_all({area}, {component}, {}, {}); } /// Drive `create_execution` and assert the typed response carries the async @@ -556,6 +569,37 @@ TEST_F(OperationHandlersFixtureTest, CancelExecutionUnknownIdReturns404) { EXPECT_EQ(result.error().code, ros2_medkit_gateway::ERR_RESOURCE_NOT_FOUND); } +// The POST executions route is registered for apps, components, areas and +// functions alike (rest_server.cpp entity-type loop), so a Location built +// from a hardcoded apps/components pair sends an areas or functions client +// into the components collection - and bypasses api_path() while doing it. +// Driven through the areas collection: create_execution validates that the +// route's entity type matches the resolved entity, so the request has to +// target a genuine non-component entity that owns the action. +// The created execution is a sub-resource of whatever collection the client +// POSTed to, so the Location must extend the request path. +TEST_F(OperationHandlersFixtureTest, CreateExecutionLocationExtendsTheRequestedCollectionPath) { + auto raw_req = make_request_with_match("/api/v1/areas/powertrain/operations/long_calibration/executions", + R"(/api/v1/areas/([^/]+)/operations/([^/]+)/executions)"); + http::TypedRequest typed(raw_req); + const std::string & requested_path = typed.path(); + dto::ExecutionCreateRequest body; + body.parameters = json{{"order", 6}}; + + auto result = handlers_->create_execution(typed, body); + + ASSERT_TRUE(result.has_value()) << result.error().code << ": " << result.error().message; + const auto * async_ptr = std::get_if(&result.value().first); + ASSERT_NE(async_ptr, nullptr); + + const auto & headers = result.value().second.headers; + auto location = std::find_if(headers.begin(), headers.end(), [](const auto & kv) { + return kv.first == "Location"; + }); + ASSERT_NE(location, headers.end()) << "202 must carry a Location header"; + EXPECT_EQ(location->second, requested_path + "/" + async_ptr->id); +} + TEST_F(OperationHandlersFixtureTest, UpdateExecutionStopReturnsAcceptedAndLocation) { const auto execution_id = create_action_execution(20); ASSERT_FALSE(execution_id.empty()); @@ -588,9 +632,14 @@ TEST_F(OperationHandlersFixtureTest, UpdateExecutionStopReturnsAcceptedAndLocati EXPECT_EQ(exec.status, "running"); EXPECT_EQ(goal_info.status, ActionGoalStatus::CANCELING); } else { - EXPECT_EQ(result.error().http_status, 400); - EXPECT_EQ(result.error().code, ros2_medkit_gateway::ERR_VENDOR_ERROR); - EXPECT_TRUE(goal_info.status == ActionGoalStatus::CANCELING || goal_info.status == ActionGoalStatus::CANCELED); + // The fixture's action server always ACCEPTS cancels, so the only + // realistic failure here is a lost/late CancelGoal response whose + // timeout could not be reconciled against the status stream: 504 + + // standard `not-responding` (issue #576). The old expectation asserted + // ERR_VENDOR_ERROR, which the handler never produced - that constant + // only ever existed on the wire after the renderer's remap. + EXPECT_EQ(result.error().http_status, 504); + EXPECT_EQ(result.error().code, ros2_medkit_gateway::ERR_NOT_RESPONDING); } } diff --git a/src/ros2_medkit_gateway/test/test_operation_manager.cpp b/src/ros2_medkit_gateway/test/test_operation_manager.cpp index 3f5ee684c..c494902b0 100644 --- a/src/ros2_medkit_gateway/test/test_operation_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_manager.cpp @@ -21,6 +21,7 @@ #include "ros2_medkit_gateway/discovery/discovery_manager.hpp" #include "ros2_medkit_gateway/ros2/transports/ros2_action_transport.hpp" #include "ros2_medkit_gateway/ros2/transports/ros2_service_transport.hpp" +#include "ros2_medkit_gateway/ros2_common/callback_groups.hpp" using namespace ros2_medkit_gateway; @@ -38,8 +39,10 @@ class TestOperationManager : public ::testing::Test { // Use short timeout for tests to avoid long waits on nonexistent services. node_ = std::make_shared("test_operation_manager_node"); discovery_manager_ = std::make_unique(node_.get()); - service_transport_ = std::make_shared(node_.get()); - action_transport_ = std::make_shared(node_.get()); + groups_ = ros2_common::create_gateway_callback_groups(*node_); + service_transport_ = std::make_shared(node_.get(), groups_.rpc_reentrant); + action_transport_ = + std::make_shared(node_.get(), groups_.rpc_reentrant, groups_.action_status); operation_manager_ = std::make_unique(service_transport_, action_transport_, discovery_manager_.get(), /*timeout=*/1); } @@ -52,7 +55,19 @@ class TestOperationManager : public ::testing::Test { node_.reset(); } + /// True while a live `/_action/status` subscription is + /// registered in the shared status callback group. Measures the stream the + /// cancel-timeout reconciliation depends on, not a bookkeeping flag. + bool has_live_status_subscription(const std::string & action_path) const { + const std::string status_topic = action_path + "/_action/status"; + return groups_.action_status->find_subscription_ptrs_if( + [&status_topic](const rclcpp::SubscriptionBase::SharedPtr & sub) { + return sub != nullptr && status_topic == sub->get_topic_name(); + }) != nullptr; + } + std::shared_ptr node_; + ros2_common::GatewayCallbackGroups groups_; std::unique_ptr discovery_manager_; std::shared_ptr service_transport_; std::shared_ptr action_transport_; @@ -420,6 +435,58 @@ TEST_F(TestOperationManager, test_unsubscribe_without_subscribe) { EXPECT_NO_THROW(operation_manager_->unsubscribe_from_action_status("/never/subscribed")); } +// A goal that is tracked for a path must always have a live status stream: +// the cancel-timeout path decides 204-vs-504 by reconciling against it, and +// cleanup decides to unsubscribe from a goal count it read earlier under a +// different lock. This drives that exact interleaving without threads - the +// emptiness observation, then a goal arriving, then the unsubscribe. +TEST_F(TestOperationManager, test_unsubscribe_keeps_stream_for_a_goal_that_arrived_meanwhile) { + const std::string action_path = "/powertrain/engine/late_goal"; + + operation_manager_->subscribe_to_action_status(action_path); + ASSERT_TRUE(has_live_status_subscription(action_path)); + + // Cleanup's view: no goals left for this path. + ASSERT_TRUE(operation_manager_->get_goals_for_action(action_path).empty()); + + // A new goal is tracked and re-subscribes (a no-op - the path is still + // registered) before cleanup gets to the unsubscribe it already decided on. + ActionGoalInfo info; + info.goal_id = "0123456789abcdef0123456789abcdef"; + info.action_path = action_path; + info.action_type = "example_interfaces/action/Fibonacci"; + info.entity_id = "engine"; + info.status = ActionGoalStatus::EXECUTING; + info.created_at = std::chrono::system_clock::now(); + info.last_update = info.created_at; + operation_manager_->inject_tracked_goal_for_testing(std::move(info)); + operation_manager_->subscribe_to_action_status(action_path); + + operation_manager_->unsubscribe_from_action_status(action_path); + + ASSERT_FALSE(operation_manager_->get_goals_for_action(action_path).empty()); + EXPECT_TRUE(has_live_status_subscription(action_path)) + << "a tracked goal lost its status stream - cancel reconciliation can never succeed for it"; +} + +// ==================== CANCEL GUARD CLASSIFICATION ==================== + +// Of the three manager-side guards, only this one is reachable through the +// contract: both HTTP entry points resolve the goal (and 404 themselves) and +// pass the resolved action_path, and every tracked goal_id comes from +// uuid_bytes_to_hex - so a malformed id never gets this far and the transport +// is never null. What a caller CAN produce is the eviction race: it finds the +// execution, the cleanup timer evicts it, and the unclassified guard tells +// the client the action server is unavailable when the truthful answer is +// "no such execution". +TEST_F(TestOperationManager, test_cancel_action_goal_not_tracked_is_not_a_transport_error) { + auto result = operation_manager_->cancel_action_goal("/test/action", "00000000000000000000000000000000"); + + EXPECT_FALSE(result.success); + EXPECT_NE(result.outcome, CancelOutcome::kTransportError) + << "an execution that is not tracked is not an action-transport failure"; +} + int main(int argc, char ** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp b/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp index b199c074d..8da965403 100644 --- a/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp @@ -15,6 +15,8 @@ #include #include +#include +#include #include #include #include @@ -97,6 +99,7 @@ class MockActionTransport : public ActionTransport { ActionCancelResult r; r.success = cancel_success_; r.return_code = cancel_return_code_; + r.outcome = cancel_success_ && cancel_return_code_ == 0 ? CancelOutcome::kOk : CancelOutcome::kErrorResponse; r.error_message = cancel_error_; return r; } @@ -118,14 +121,32 @@ class MockActionTransport : public ActionTransport { void subscribe_status(const std::string & action_path, StatusCallback callback) override { subscribed_paths_.push_back(action_path); + // Idempotent, exactly like Ros2ActionTransport::subscribe_status: while a + // subscription for the path still exists, re-subscribing keeps the first + // callback and creates nothing. A double that silently re-armed itself + // here could not observe the window this models. + if (callbacks_.count(action_path) > 0) { + return; + } callbacks_[action_path] = std::move(callback); } void unsubscribe_status(const std::string & action_path) override { + // One-shot re-entry: stand in for an HTTP worker that lands between the + // manager releasing subscriptions_mutex_ and this call. Production reaches + // exactly this interleaving - the manager holds no lock here. + if (reentry_ != nullptr) { + auto reentry = std::move(reentry_); + reentry_ = nullptr; + reentry(); + } unsubscribed_paths_.push_back(action_path); callbacks_.erase(action_path); } + /// Runs once, inside the next unsubscribe_status call, before it erases. + std::function reentry_; + /// Test helper to fire a status update on a previously subscribed path. void fire_status(const std::string & action_path, const std::string & goal_id, ActionGoalStatus status) { auto it = callbacks_.find(action_path); @@ -278,6 +299,9 @@ TEST(OperationManagerRoutingTest, CancelActionGoalRoutesToActionTransport) { EXPECT_EQ(act->cancel_calls_, 1); EXPECT_EQ(act->last_cancel_path_, "/p/a"); EXPECT_EQ(act->last_cancel_goal_id_, sent.goal_id); + // The cancel budget must follow the configured service timeout exactly, + // like every other action RPC - no hidden floor (issue #576). + EXPECT_DOUBLE_EQ(act->last_cancel_timeout_, 2.0); auto tracked = mgr.get_tracked_goal(sent.goal_id); ASSERT_TRUE(tracked.has_value()); @@ -386,4 +410,50 @@ TEST(OperationManagerRoutingTest, GetLatestGoalForActionPicksMostRecent) { EXPECT_EQ(latest->goal_id, second.goal_id); } +// The unsubscribe veto only helps if it holds for the whole operation. The +// manager releases subscriptions_mutex_ before calling the transport, so a +// goal sent into that gap re-flags the path (a no-op at the transport, whose +// subscription still exists) and then has that subscription destroyed by the +// unsubscribe already in flight. End state: goal tracked, manager believes the +// path is subscribed, no status stream - and nothing repairs it, because +// subscribe_to_action_status short-circuits on the flag. That is precisely the +// state the cancel-timeout reconciliation cannot survive. +TEST(OperationManagerRoutingTest, UnsubscribeKeepsTheStreamForAGoalThatArrivesDuringTheTransportCall) { + auto svc = std::make_shared(); + auto act = std::make_shared(); + OperationManager mgr(svc, act, nullptr, /*timeout=*/2); + const std::string path = "/p/a"; + const std::string late_goal = "00000000000000000000000000000009"; + + // A path that was subscribed and whose goals have all just been cleaned up. + mgr.subscribe_to_action_status(path); + ASSERT_TRUE(mgr.get_goals_for_action(path).empty()); + + act->reentry_ = [&mgr, &path, &late_goal]() { + ActionGoalInfo info; + info.goal_id = late_goal; + info.action_path = path; + info.action_type = "example_interfaces/action/Fibonacci"; + info.entity_id = "engine"; + info.status = ActionGoalStatus::EXECUTING; + info.created_at = std::chrono::system_clock::now(); + info.last_update = info.created_at; + mgr.inject_tracked_goal_for_testing(std::move(info)); + mgr.subscribe_to_action_status(path); + }; + + mgr.unsubscribe_from_action_status(path); + + ASSERT_FALSE(mgr.get_goals_for_action(path).empty()) << "the late goal must still be tracked"; + EXPECT_EQ(act->callbacks_.count(path), 1u) << "a tracked goal lost its status stream"; + + // The stream must still reach the tracking map - a live callback that is + // never invoked would be an equally dead stream. + act->fire_status(path, late_goal, ActionGoalStatus::CANCELED); + auto tracked = mgr.get_tracked_goal(late_goal); + ASSERT_TRUE(tracked.has_value()); + EXPECT_EQ(tracked->status, ActionGoalStatus::CANCELED) + << "status frames no longer reach a goal the manager believes is subscribed"; +} + } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py index b25b3bc20..018949d52 100644 --- a/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py +++ b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py @@ -22,6 +22,50 @@ DEFAULT_BASE_URL = f'http://localhost:{DEFAULT_PORT}{API_BASE_PATH}' +def get_time_scale(): + """Return the multiplier for in-test wall-clock budgets. + + CTest timeouts are stretched by the sanitizer CI jobs (a generic x3 + rewrite of every declared ``TIMEOUT``), but budgets asserted *inside* a + test are invisible to that rewrite: an ASan/TSan-instrumented gateway can + blow a hard-coded "must answer within N seconds" assertion long before + ctest's own clock runs out, and the failure then reads as a regression + rather than as sanitizer overhead. Those jobs set + ``MEDKIT_TEST_TIME_SCALE`` to the same factor they apply to ctest. + + Unset / unparseable / below 1.0 means "no scaling", so the normal jobs + keep the tight budgets that give the assertions their falsifying power. + """ + try: + scale = float(os.environ.get('MEDKIT_TEST_TIME_SCALE', '1')) + except ValueError: + return 1.0 + return scale if scale >= 1.0 else 1.0 + + +def sanitizers_enabled(): + """Return True when running against a sanitizer-instrumented build. + + Some resources are affordable in a normal build and not under ASan/TSan - + most obviously threads, which each carry shadow and history state and which + the runtime has to tear down one by one. A test that sizes such a resource + at its documented maximum can exceed launch_testing's shutdown grace period + under instrumentation and have the process SIGKILLed, which then reads as a + product failure rather than as instrumentation cost. + + Detected from the sanitizer runtimes' own configuration variables, which + the ASan and TSan jobs already set, so a future sanitizer job cannot forget + to opt in. ``MEDKIT_TEST_SANITIZED`` forces it on for a local reproduction + (any value except empty / ``0`` / ``false``). + """ + forced = os.environ.get('MEDKIT_TEST_SANITIZED', '').strip() + if forced and forced.lower() not in ('0', 'false'): + return True + return any( + os.environ.get(name) for name in ('ASAN_OPTIONS', 'TSAN_OPTIONS', 'UBSAN_OPTIONS') + ) + + def get_test_port(offset=0): """Return the assigned test port plus an optional offset. diff --git a/src/ros2_medkit_integration_tests/test/features/test_action_cancel_unavailable.test.py b/src/ros2_medkit_integration_tests/test/features/test_action_cancel_unavailable.test.py new file mode 100644 index 000000000..e8cbe5e07 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_action_cancel_unavailable.test.py @@ -0,0 +1,196 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Cancel against a dead action server maps to 503, not "rejected" (issue #576). + +When the action server behind a tracked execution dies, its +``_action/cancel_goal`` service leaves the ROS graph, so a DELETE on the +execution cannot even deliver the cancel request. That is an availability +failure, not a rejection by the server - the gateway must answer +``503`` + ``x-medkit-ros2-action-unavailable``, not the definitive +``400`` + ``x-medkit-ros2-action-rejected`` it used to conflate every +non-success into. + +The action server is spawned as a raw subprocess (not a launch action) so +the test can terminate it mid-flight, mirroring the spawn pattern of +``test_graph_event_discovery``. +""" + +import os +import signal +import subprocess +import unittest + +from ament_index_python.packages import get_package_prefix +from launch import LaunchDescription +import launch_testing +import launch_testing.actions +import requests + +from ros2_medkit_test_utils.constants import ( + ALLOWED_EXIT_CODES, + DEFAULT_DOMAIN_ID, +) +from ros2_medkit_test_utils.coverage import get_coverage_env +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import ( + create_gateway_node, + DEMO_NODE_REGISTRY, +) + +APP_ID = 'long_calibration' +OPERATION_ID = 'long_calibration' +ENTITY_ENDPOINT = '/apps/long_calibration' + + +def generate_test_description(): + # Gateway only - the action server is spawned per-test as a subprocess so + # it can be killed mid-flight. + gateway_node = create_gateway_node() + return ( + LaunchDescription([ + gateway_node, + launch_testing.actions.ReadyToTest(), + ]), + {'gateway_node': gateway_node}, + ) + + +def _resolve_demo_executable(name): + """Resolve a demo executable to its installed absolute path.""" + pkg = 'ros2_medkit_integration_tests' + prefix = get_package_prefix(pkg) + candidate = os.path.join(prefix, 'lib', pkg, name) + if not os.path.isfile(candidate): + raise FileNotFoundError(f'demo executable not found: {candidate}') + return candidate + + +def _terminate_process(proc): + if proc is None or proc.poll() is not None: + return + proc.send_signal(signal.SIGTERM) + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait(timeout=5) + + +class TestActionCancelUnavailable(GatewayTestCase): + """DELETE on an execution whose action server died answers 503.""" + + MIN_EXPECTED_APPS = 0 # discovery is polled per-test after the spawn + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls._action_proc = None + + @classmethod + def tearDownClass(cls): + _terminate_process(cls._action_proc) + cls._action_proc = None + super().tearDownClass() + + @classmethod + def _spawn_action_server(cls): + executable, ros_name, namespace = DEMO_NODE_REGISTRY['long_calibration'] + env = os.environ.copy() + env['ROS_DOMAIN_ID'] = str(DEFAULT_DOMAIN_ID) + env.update(get_coverage_env('ros2_medkit_integration_tests')) + binary = _resolve_demo_executable(executable) + return subprocess.Popen( + [ + binary, + '--ros-args', + '-r', f'__ns:={namespace}', + '-r', f'__node:={ros_name}', + ], + env=env, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + + def test_cancel_after_action_server_death_returns_503(self): + """Kill the action server post-accept; DELETE maps to 503 unavailable.""" + cls = type(self) + cls._action_proc = self._spawn_action_server() + + # Wait for the operation to be USABLE (resolved interface type), then + # start a long-running goal the server will never finish. + self.wait_for_operation_type_info( + ENTITY_ENDPOINT, OPERATION_ID, + ('goal', 'result', 'feedback'), max_wait=30.0, + ) + _, data = self.create_execution( + ENTITY_ENDPOINT, OPERATION_ID, input_data={'order': 45}, + ) + execution_id = data['id'] + exec_endpoint = ( + f'{ENTITY_ENDPOINT}/operations/{OPERATION_ID}' + f'/executions/{execution_id}' + ) + + # The execution must be registered before the server goes away. + status_data = self.poll_endpoint(exec_endpoint, timeout=10.0, interval=0.3) + self.assertIn(status_data['status'], ['running', 'completed']) + + # Terminate the action server (SIGTERM: the participant leaves the + # graph immediately, so the cancel service disappears deterministically + # instead of lingering for a DDS liveliness lease). + _terminate_process(cls._action_proc) + cls._action_proc = None + + # Wait until the gateway's view of the graph has dropped the app - + # after this, the cancel_goal service is guaranteed gone as well. + self.poll_endpoint_until( + '/apps', + lambda d: d if not any( + app.get('id') == APP_ID for app in d.get('items', []) + ) else None, + timeout=30.0, + interval=0.5, + ) + + # Cancel now cannot reach any server: availability failure, not a + # rejection. The gateway waits up to 2s for the service before + # answering, so give the client comfortable headroom. + response = requests.delete( + f'{self.BASE_URL}{exec_endpoint}', timeout=30, + ) + self.assertEqual( + response.status_code, 503, + f'Expected 503 for cancel against a dead action server, got ' + f'{response.status_code}: {response.text}', + ) + body = response.json() + self.assertEqual(body.get('error_code'), 'vendor-error') + self.assertEqual( + body.get('vendor_code'), 'x-medkit-ros2-action-unavailable', + f'Expected the availability vendor code, got: {body}', + ) + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + + def test_exit_codes(self, proc_info): + """All processes exit cleanly.""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}', + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_action_status_first_goal.test.py b/src/ros2_medkit_integration_tests/test/features/test_action_status_first_goal.test.py new file mode 100644 index 000000000..1aa012e51 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_action_status_first_goal.test.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""The `/_action/status` stream must carry the FIRST goal on a path (issue #576). + +Since the cancel-timeout work, the action status stream is the gateway's only +source of truth for a goal's terminal state: `update_goal_status` refuses to +leave a terminal status, `map_cancel_result` decides 204-vs-504 by reading it, +and `get_action_result` has no production caller, so nothing else can ever +correct a goal's status. + +The stream is subscribed lazily - `send_action_goal` tracks the goal and only +then calls `subscribe_to_action_status` - so on the first goal for a path the +DDS subscription is still matching while the action is already running. An +action that finishes inside that window publishes its terminal status to a +reader that does not exist yet. Whether the gateway ever learns the outcome +then depends entirely on the subscription's durability: a VOLATILE reader is +delivered nothing on match, a TRANSIENT_LOCAL one receives the writer's last +sample, which is exactly the terminal frame it missed. + +`Fibonacci(order=1)` runs zero loop iterations in the demo action, so it +terminates essentially at accept time - the window this test needs. The +execution is the first goal of a freshly launched gateway, so no earlier +subscription can mask the effect. +""" + +import unittest + +import launch_testing +import requests + +from ros2_medkit_test_utils.constants import ( + ALLOWED_EXIT_CODES, + API_BASE_PATH, + get_test_port, + get_time_scale, +) +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + +# Zero loop iterations in demo_long_calibration_action: the goal succeeds +# immediately after it is accepted. +IMMEDIATE_ORDER = 1 + +# The action itself needs milliseconds. This budget covers DDS subscription +# matching plus the gateway's own tracking, and is deliberately far larger +# than either so a failure means "never arrived", not "arrived late". +TERMINAL_STATUS_BUDGET_SEC = 20.0 * get_time_scale() + +TERMINAL_ROS2_STATUSES = {'succeeded', 'canceled', 'aborted'} + + +def generate_test_description(): + return create_test_launch( + demo_nodes=['long_calibration'], + fault_manager=False, + ) + + +class TestActionStatusFirstGoal(GatewayTestCase): + """A goal that terminates during subscription matching still reports terminal.""" + + MIN_EXPECTED_APPS = 1 + REQUIRED_APPS = {'long_calibration'} + + def test_first_goal_that_completes_immediately_reports_terminal_status(self): + """The gateway must learn the outcome of the first goal on a path. + + Without it the execution is pinned at `executing` forever: no further + status frame is published, no RPC re-reads the goal, a timed-out cancel + can never reconcile to 204, and at 2x max_age the goal is force-evicted + with a "server crashed" warning naming a server that did its job. + """ + self.wait_for_operation('/apps/long_calibration', 'long_calibration', max_wait=45.0) + + created_response = requests.post( + f'{self.BASE_URL}/apps/long_calibration/operations/long_calibration/executions', + json={'parameters': {'order': IMMEDIATE_ORDER}}, + timeout=10, + ) + self.assertEqual(created_response.status_code, 202, created_response.text) + execution_id = created_response.json()['id'] + + endpoint = ( + f'/apps/long_calibration/operations/long_calibration/executions/{execution_id}' + ) + self.assertEqual( + created_response.headers.get('Location'), f'{API_BASE_PATH}{endpoint}', + 'the 202 Location must be the absolute path of the execution resource', + ) + # Follow the Location the server handed out, exactly as a client would: + # a prefix regression makes this 404 while a test that compares the + # handler's output to its own input stays green. + followed = requests.get( + f"http://localhost:{get_test_port()}{created_response.headers['Location']}", timeout=10 + ) + self.assertEqual(followed.status_code, 200, followed.text) + self.assertEqual(followed.json().get('capability'), 'execute') + + def terminal_or_none(d): + status = (d.get('x-medkit') or {}).get('ros2_status') + return d if status in TERMINAL_ROS2_STATUSES else None + + final = self.poll_endpoint_until( + endpoint, + terminal_or_none, + timeout=TERMINAL_STATUS_BUDGET_SEC, + interval=0.3, + ) + self.assertEqual( + (final.get('x-medkit') or {}).get('ros2_status'), 'succeeded', + f'the immediate goal should be reported as succeeded: {final}', + ) + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + + def test_exit_codes(self, proc_info): + """All processes exit cleanly.""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}', + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_executor_single_thread.test.py b/src/ros2_medkit_integration_tests/test/features/test_executor_single_thread.test.py new file mode 100644 index 000000000..805ca36be --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_executor_single_thread.test.py @@ -0,0 +1,110 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Single-executor-thread guard for the RPC callback-group split (issue #575). + +The RPC response callbacks live in a shared Reentrant callback group so a +second executor thread can dispatch them while the default group is busy. +A Reentrant group must NOT *require* a second thread: with +``server.executor_threads: 1`` and nothing hogging the executor, service +calls, action goals, and action cancels must all still complete - the single +thread services both groups. This guards against a fix that accidentally +made RPC dispatch depend on a dedicated thread (which would deadlock every +single-threaded deployment). +""" + +import unittest + +import launch_testing +import requests + +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + + +def generate_test_description(): + return create_test_launch( + demo_nodes=['calibration', 'long_calibration'], + fault_manager=False, + gateway_params={ + 'server.executor_threads': 1, + }, + ) + + +class TestExecutorSingleThread(GatewayTestCase): + """All operation flows complete on a one-thread executor (no deadlock).""" + + MIN_EXPECTED_APPS = 2 + REQUIRED_APPS = {'calibration', 'long_calibration'} + + ACTION_ENDPOINT = '/apps/long_calibration' + ACTION_OPERATION = 'long_calibration' + + def test_service_operation_completes(self): + """A synchronous service-backed operation completes on one thread.""" + self.wait_for_operation('/apps/calibration', 'calibrate') + + response = requests.post( + f'{self.BASE_URL}/apps/calibration/operations/calibrate/executions', + json={'parameters': {}}, + timeout=30, + ) + self.assertEqual( + response.status_code, 200, + f'Service operation failed with executor_threads=1 ' + f'(HTTP {response.status_code}): {response.text}', + ) + + def test_action_execute_and_cancel_completes(self): + """Action goal send + mid-flight cancel complete on one thread.""" + self.wait_for_operation_type_info( + self.ACTION_ENDPOINT, self.ACTION_OPERATION, + ('goal', 'result', 'feedback'), + ) + + response, data = self.create_execution( + self.ACTION_ENDPOINT, self.ACTION_OPERATION, + input_data={'order': 20}, + ) + self.assertEqual(response.status_code, 202) + execution_id = data['id'] + + exec_endpoint = ( + f'{self.ACTION_ENDPOINT}/operations/' + f'{self.ACTION_OPERATION}/executions/{execution_id}' + ) + status_data = self.poll_endpoint(exec_endpoint, timeout=10.0, interval=0.3) + self.assertIn(status_data['status'], ['running', 'completed']) + + response = self.delete_request( + exec_endpoint, + timeout=25, + expected_status=204, + ) + self.assertEqual(len(response.content), 0) + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + + def test_exit_codes(self, proc_info): + """All processes exit cleanly.""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}', + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py b/src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py new file mode 100644 index 000000000..5e7429e8a --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Executor-starvation falsifier for the RPC callback-group split (issue #575). + +The gateway's generic service clients complete their response futures via +executor callbacks. When those callbacks live in the node's default +MutuallyExclusive callback group - the same group as the discovery refresh +timers - one long refresh pass stalls every in-flight RPC response no matter +how many executor threads are configured. This test manufactures that stall +deterministically and asserts a service-backed operation still completes +fast, which requires the response callback to run in the shared Reentrant +RPC group where a second executor thread can dispatch it: + +- Aggregation is enabled with a single peer pointing at an RFC 5737 + TEST-NET-1 address (192.0.2.1). TCP connects to that address are + black-holed (SYN never answered), so every peer health check inside + ``refresh_cache()`` blocks the refresh timer callback for the full + health budget (capped at 1s inside ``PeerClient``) instead of failing + fast. ``test_blackhole_peer_actually_stalls`` guards that precondition. +- ``refresh_interval_ms`` is set well below that stall, so the backstop + refresh timer is permanently past due and refresh passes run back to + back: the default callback group is continuously occupied by refresh + work for the lifetime of the test. +- A service-backed operation is executed mid-stall. Its response can only + be delivered by an executor callback. With the response callbacks in the + shared Reentrant RPC group, a second executor thread dispatches the + response immediately; with them in the default group, the response sits + undeliverable until the 10s service budget expires and the gateway + reports a bogus "Service call timed out". +""" + +import socket +import time +import unittest + +import launch_testing +import requests + +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES, get_time_scale +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + +# RFC 5737 TEST-NET-1: guaranteed unassigned; TCP connects are black-holed +# (no SYN-ACK, no RST), so the peer health check blocks for its full budget. +BLACKHOLE_PEER_HOST = '192.0.2.1' +BLACKHOLE_PEER_PORT = 9100 +BLACKHOLE_PEER_URL = f'http://{BLACKHOLE_PEER_HOST}:{BLACKHOLE_PEER_PORT}' + +# Well below the ~1s per-pass health-check stall so the backstop refresh +# timer is always past due and a stalled pass is always in flight. +REFRESH_INTERVAL_MS = 200 + +# The gateway's service budget is 10s (service_call_timeout_sec default). +# The pre-fix behaviour was to burn the whole budget and fail with 500; +# the fixed gateway answers in well under a second even mid-stall. 8s keeps +# CI headroom while staying decisively below the failure mode. +# +# Scaled by MEDKIT_TEST_TIME_SCALE so the sanitizer jobs - which multiply +# every ctest timeout but cannot reach an assertion inside a test - do not +# read their own instrumentation overhead as a starved response. The +# unscaled value stays 8.0 so the falsifier keeps its edge where it was +# proven red (main burns the full 10s budget and answers 500, which no +# scaling can turn into a pass). +FAST_COMPLETION_BUDGET_SEC = 8.0 * get_time_scale() + + +def generate_test_description(): + return create_test_launch( + demo_nodes=['calibration'], + fault_manager=False, + gateway_params={ + 'refresh_interval_ms': REFRESH_INTERVAL_MS, + 'aggregation.enabled': True, + 'aggregation.timeout_ms': 5000, + 'aggregation.announce': False, + 'aggregation.discover': False, + 'aggregation.peer_urls': [BLACKHOLE_PEER_URL], + 'aggregation.peer_names': ['blackhole_peer'], + }, + ) + + +class TestExecutorStarvation(GatewayTestCase): + """A blocking-RPC response is dispatched while refresh hogs the default group.""" + + MIN_EXPECTED_APPS = 1 + REQUIRED_APPS = {'calibration'} + + def test_blackhole_peer_actually_stalls(self): + """Precondition: this environment must black-hole TEST-NET-1 connects. + + If the network rejects 192.0.2.1 quickly (ICMP unreachable / RST), + the refresh pass never stalls and the starvation assertion below + would pass vacuously on broken code. Fail loudly in that case so + the fixture gets fixed rather than silently proving nothing. + """ + start = time.monotonic() + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.settimeout(2.0) + try: + sock.connect((BLACKHOLE_PEER_HOST, BLACKHOLE_PEER_PORT)) + except socket.timeout: + pass # expected: SYN black-holed until our own timeout fires + except OSError: + pass # fast failure - caught by the elapsed assertion below + finally: + sock.close() + elapsed = time.monotonic() - start + self.assertGreater( + elapsed, 1.5, + f'TEST-NET-1 connect failed after only {elapsed:.2f}s instead of ' + f'black-holing - this environment cannot manufacture the refresh ' + f'stall, so the starvation falsifier would be vacuous', + ) + + def test_service_operation_completes_during_refresh_stall(self): + """A service-backed operation must not be starved by refresh passes. + + The measured request runs while refresh passes (each stalled ~1s on + the black-hole peer health check) occupy the default callback group + back to back. Only the operation call itself is timed - discovery + data lands late here because every pass includes the stall, so + readiness gets its own generous poll first. + """ + self.wait_for_operation('/apps/calibration', 'calibrate', max_wait=45.0) + + start = time.monotonic() + response = requests.post( + f'{self.BASE_URL}/apps/calibration/operations/calibrate/executions', + json={'parameters': {}}, + timeout=30, + ) + elapsed = time.monotonic() - start + + self.assertEqual( + response.status_code, 200, + f'Service-backed operation failed during the refresh stall ' + f'(HTTP {response.status_code} after {elapsed:.1f}s): ' + f'{response.text}', + ) + self.assertLess( + elapsed, FAST_COMPLETION_BUDGET_SEC, + f'Operation took {elapsed:.1f}s - the service response was ' + f'starved behind the stalled default callback group instead of ' + f'being dispatched by a second executor thread', + ) + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + + def test_exit_codes(self, proc_info): + """All processes exit cleanly.""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}', + ) diff --git a/src/ros2_medkit_integration_tests/test/features/test_thread_pool_starvation.test.py b/src/ros2_medkit_integration_tests/test/features/test_thread_pool_starvation.test.py index 9d2f45cf9..f8df78c7d 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_thread_pool_starvation.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_thread_pool_starvation.test.py @@ -24,17 +24,33 @@ - the bounded `executor_threads` value is actually applied (the gateway logs the thread count, so it is observable rather than merely set). -Both checks read the gateway's own process output, so they are deterministic and +It additionally sweeps the documented ``server.executor_threads`` clamp range +``[1, 256]`` (issue #575): three extra gateways launch with the range floor, +the range ceiling, and an out-of-range value, and each must log the resolved +(clamped) count. Without the endpoint sweep, a regression in the clamp (or in +the parameter read) would only surface for mid-range values. + +All checks read the gateways' own process output, so they are deterministic and do not depend on request timing. """ +import time import unittest +from launch import LaunchDescription import launch_testing - -from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +import launch_testing.actions +import requests + +from ros2_medkit_test_utils.constants import ( + ALLOWED_EXIT_CODES, + API_BASE_PATH, + get_test_port, + get_time_scale, + sanitizers_enabled, +) from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase -from ros2_medkit_test_utils.launch_helpers import create_test_launch +from ros2_medkit_test_utils.launch_helpers import create_gateway_node # Pool below the shipped budget: 2 < sse.max_clients(2) + cold_wait_cap(4) = 6. # executor_threads is set to a non-default value so the "applied" assertion is @@ -43,25 +59,84 @@ SSE_MAX_CLIENTS = 2 EXECUTOR_THREADS = 3 +# Clamp sweep for server.executor_threads (documented range [1, 256]). +# The out-of-range value must clamp to the nearest endpoint; 300 clamps to the +# ceiling, so its expected log line differs from the floor gateway's and the +# three assertions stay unambiguous even without per-process scoping. +EXECUTOR_THREADS_FLOOR = 1 +EXECUTOR_THREADS_CEILING = 256 +EXECUTOR_THREADS_INVALID = 0 # below the floor -> must clamp to 1 + +# The upper endpoint is exercised at its documented value in a normal build and +# at a smaller one under a sanitizer. Not a convenience: a TSan-instrumented +# gateway with 256 executor threads did not finish shutting down inside +# launch_testing's grace period, so it was escalated SIGINT -> SIGTERM -> +# SIGKILL and the suite reported exit code -9 for a gateway that had started, +# logged its bound and served /health correctly. Unsanitized the same teardown +# costs nothing measurable (the whole test runs in ~1.3 s), so the cost is +# instrumentation, not the gateway. +# +# CONSEQUENCE, stated so nobody reads more coverage into a green sanitizer run +# than it has: under a sanitizer this file does NOT pin the documented ceiling. +# It pins that a large, genuinely multi-threaded executor is accepted and +# serves. The ceiling itself is pinned by every normal build - jazzy, humble, +# lyrical, coverage and Pixi all run this test unsanitized. +SANITIZED_EXECUTOR_THREADS_CEILING = 16 +CEILING_UNDER_TEST = ( + SANITIZED_EXECUTOR_THREADS_CEILING if sanitizers_enabled() else EXECUTOR_THREADS_CEILING +) + def generate_test_description(): - return create_test_launch( - demo_nodes=[], # no discovery needed - this is a config/log test - fault_manager=False, - gateway_params={ + gateway_node = create_gateway_node( + extra_params={ 'server.http_thread_pool_size': HTTP_THREAD_POOL_SIZE, 'server.executor_threads': EXECUTOR_THREADS, 'sse.max_clients': SSE_MAX_CLIENTS, }, ) + # Clamp-endpoint gateways (issue #575): distinct node names and ports so + # they can share the launch. Only their startup logs are asserted. + gw_floor = create_gateway_node( + port=get_test_port(1), + name='gateway_threads_floor', + extra_params={'server.executor_threads': EXECUTOR_THREADS_FLOOR}, + ) + gw_ceiling = create_gateway_node( + port=get_test_port(2), + name='gateway_threads_ceiling', + extra_params={'server.executor_threads': CEILING_UNDER_TEST}, + ) + gw_invalid = create_gateway_node( + port=get_test_port(3), + name='gateway_threads_invalid', + extra_params={'server.executor_threads': EXECUTOR_THREADS_INVALID}, + ) + + return ( + LaunchDescription([ + gateway_node, + gw_floor, + gw_ceiling, + gw_invalid, + launch_testing.actions.ReadyToTest(), + ]), + { + 'gateway_node': gateway_node, + 'gw_floor': gw_floor, + 'gw_ceiling': gw_ceiling, + 'gw_invalid': gw_invalid, + }, + ) + class TestThreadPoolStarvationGuard(GatewayTestCase): """An under-budget pool is flagged, and the executor bound is observable.""" - MIN_EXPECTED_APPS = 0 # skip discovery waiting; the gateway only needs to start + MIN_EXPECTED_APPS = 0 # skip discovery waiting; the gateways only need to start - def test_startup_warns_when_pool_below_budget(self, proc_output): + def test_startup_warns_when_pool_below_budget(self, proc_output, gateway_node): """The gateway warns when http_thread_pool_size < max_clients + cold_wait_cap. Without this warning the misconfiguration is silent: a burst of SSE @@ -69,10 +144,10 @@ def test_startup_warns_when_pool_below_budget(self, proc_output): operator-facing guard, so we assert it is actually emitted. """ proc_output.assertWaitFor( - 'is below sse.max_clients', timeout=15, + 'is below sse.max_clients', process=gateway_node, timeout=15, ) - def test_executor_thread_bound_is_applied(self, proc_output): + def test_executor_thread_bound_is_applied(self, proc_output, gateway_node): """The configured executor_threads value is honoured (and observable). The reviewer noted executor_threads was set in tests but never observed. @@ -81,7 +156,62 @@ def test_executor_thread_bound_is_applied(self, proc_output): log a different number. """ proc_output.assertWaitFor( - f'Main executor bounded to {EXECUTOR_THREADS} threads', timeout=15, + f'Main executor bounded to {EXECUTOR_THREADS} threads', + process=gateway_node, timeout=15, + ) + + def test_executor_threads_floor_applied(self, proc_output, gw_floor): + """The documented range floor (1) is accepted and applied as-is.""" + proc_output.assertWaitFor( + 'Main executor bounded to 1 threads', process=gw_floor, timeout=15, + ) + + def test_executor_threads_ceiling_applied(self, proc_output, gw_ceiling): + """The range ceiling is accepted and applied as-is. + + `CEILING_UNDER_TEST` is the documented 256 in a normal build and a + smaller value under a sanitizer - see the constant for why. + """ + proc_output.assertWaitFor( + f'Main executor bounded to {CEILING_UNDER_TEST} threads', + process=gw_ceiling, timeout=15, + ) + + def test_executor_threads_ceiling_gateway_serves_requests(self): + """The high-thread-count gateway actually serves, not just logs its bound. + + Every other assertion in the clamp sweep reads a log line, so a + regression that logged the clamped count and then failed to bring the + executor up would stay green. One real request over HTTP pins that + the clamped value was applied to a working gateway. + """ + url = f'http://localhost:{get_test_port(2)}{API_BASE_PATH}/health' + deadline = time.monotonic() + 30.0 * get_time_scale() + last_error = None + while time.monotonic() < deadline: + try: + response = requests.get(url, timeout=5 * get_time_scale()) + if response.status_code == 200: + self.assertEqual(response.json().get('status'), 'healthy') + return + last_error = f'HTTP {response.status_code}: {response.text}' + except requests.RequestException as exc: + last_error = repr(exc) + time.sleep(0.5) + self.fail( + f'the executor_threads={CEILING_UNDER_TEST} gateway never served ' + f'/health: {last_error}' + ) + + def test_executor_threads_invalid_clamps_to_floor(self, proc_output, gw_invalid): + """An out-of-range value (0) clamps to the floor instead of breaking. + + 0 would mean "all host cores" to rclcpp (footprint regression) or an + empty pool to a naive reader; the documented behaviour is a clamp to + the [1, 256] range, observable through the same startup log line. + """ + proc_output.assertWaitFor( + 'Main executor bounded to 1 threads', process=gw_invalid, timeout=15, ) diff --git a/src/ros2_medkit_integration_tests/test/scenarios/test_scenario_action_lifecycle.test.py b/src/ros2_medkit_integration_tests/test/scenarios/test_scenario_action_lifecycle.test.py index ba4ab5ad8..25fbf1c5a 100644 --- a/src/ros2_medkit_integration_tests/test/scenarios/test_scenario_action_lifecycle.test.py +++ b/src/ros2_medkit_integration_tests/test/scenarios/test_scenario_action_lifecycle.test.py @@ -26,7 +26,7 @@ import launch_testing.actions import requests -from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES, get_time_scale from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase from ros2_medkit_test_utils.launch_helpers import create_test_launch @@ -146,8 +146,9 @@ def test_02_cancel_action_execution(self): # Cancel the execution. The client budget must exceed the gateway's: cancelling is # a service round-trip to the action server (up to 2s to find the service plus the - # cancel budget itself), so a 10s client timeout would surface a slow-but-successful - # cancel as an opaque ReadTimeout instead of the server's own diagnosable error. + # configured service_call_timeout_sec, default 10s), so a 10s client timeout would + # surface a slow-but-successful cancel as an opaque ReadTimeout instead of the + # server's own diagnosable error. response = self.delete_request( self._exec_endpoint(execution_id), timeout=25, @@ -155,6 +156,22 @@ def test_02_cancel_action_execution(self): ) self.assertEqual(len(response.content), 0) + # The /_action/status stream is the authority for the goal's state, + # and the accepted-cancel reply raced it: whichever of the two the + # gateway processed second used to win. When the stream's terminal + # frame lost that race the execution was pinned at "canceling" + # forever - no further frame is ever published - so this is the only + # place the defect is visible end to end. Any terminal state is fine + # (the goal may also have completed before the cancel landed); what + # must not happen is the execution never leaving a running state. + terminal = {'canceled', 'succeeded', 'aborted'} + self.poll_endpoint_until( + self._exec_endpoint(execution_id), + lambda d: d if (d.get('x-medkit') or {}).get('ros2_status') in terminal else None, + timeout=20.0 * get_time_scale(), + interval=0.3, + ) + def test_03_service_execution_returns_immediately(self): """Create a Trigger service execution, get immediate result. diff --git a/src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp b/src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp index 39ef92379..5499f250b 100644 --- a/src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp +++ b/src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp @@ -89,7 +89,7 @@ class LogBridgeNode : public rclcpp::Node { /// (first occurrence passes; same code+severity within report_cooldown_sec is /// suppressed; 0.0 disables). Keyed by severity so a WARN never suppresses a /// same-message ERROR escalation. Exposed for unit testing. - bool cooldown_allows(const std::string & fault_code, uint8_t severity, rclcpp::Time now); + bool cooldown_allows(const std::string & fault_code, uint8_t severity, const rclcpp::Time & now); /// Fetch (or lazily create) the per-source FaultReporter for an originating /// node, so the fault's source_id is the node that logged, not the bridge. diff --git a/src/ros2_medkit_log_bridge/src/log_bridge_node.cpp b/src/ros2_medkit_log_bridge/src/log_bridge_node.cpp index 6b5678fc3..e66d45292 100644 --- a/src/ros2_medkit_log_bridge/src/log_bridge_node.cpp +++ b/src/ros2_medkit_log_bridge/src/log_bridge_node.cpp @@ -16,9 +16,11 @@ #include #include +#include #include #include #include +#include #include #include "ros2_medkit_msgs/msg/fault.hpp" @@ -65,13 +67,15 @@ void LogBridgeNode::load_parameters() { // Default floor is WARN. WARN passes through each node's FaultReporter // LocalFilter (threshold/window debounce); ERROR/FATAL bypass it. Raise to 40 // (ERROR) on chatty / constrained targets to cut volume. - const int floor = declare_parameter("severity_floor", kLevelWarn); + // ROS parameters are int64; read them as such and narrow only after the + // clamp, so an out-of-range value cannot wrap on the way in. + const int64_t floor = declare_parameter("severity_floor", static_cast(kLevelWarn)); // int -> uint8_t silently wraps; clamp so a bad value cannot pass everything. if (floor < 0 || floor > kLevelFatal) { - RCLCPP_WARN(get_logger(), "severity_floor=%d out of range [0,%u], clamping", floor, + RCLCPP_WARN(get_logger(), "severity_floor=%" PRId64 " out of range [0,%u], clamping", floor, static_cast(kLevelFatal)); } - severity_floor_ = static_cast(std::clamp(floor, 0, static_cast(kLevelFatal))); + severity_floor_ = static_cast(std::clamp(floor, INT64_C(0), static_cast(kLevelFatal))); // Normalize the prefix so a non-conforming value cannot yield a fault_code // violating medkit's [A-Z0-9_] charset. code_prefix_ = to_upper_snake(declare_parameter("code_prefix", "LOG"), 32); @@ -80,10 +84,9 @@ void LogBridgeNode::load_parameters() { } exclude_nodes_ = declare_parameter>("exclude_nodes", std::vector{}); include_only_nodes_ = declare_parameter>("include_only_nodes", std::vector{}); - max_tracked_nodes_ = declare_parameter("max_tracked_nodes", 512); - if (max_tracked_nodes_ < 1) { - max_tracked_nodes_ = 1; - } + const int64_t max_tracked_nodes = declare_parameter("max_tracked_nodes", INT64_C(512)); + max_tracked_nodes_ = static_cast( + std::clamp(max_tracked_nodes, INT64_C(1), static_cast(std::numeric_limits::max()))); report_cooldown_sec_ = declare_parameter("report_cooldown_sec", 5.0); if (report_cooldown_sec_ < 0.0) { report_cooldown_sec_ = 0.0; @@ -177,7 +180,7 @@ bool LogBridgeNode::node_is_eligible(const std::string & source_id) const { return true; } -bool LogBridgeNode::cooldown_allows(const std::string & fault_code, uint8_t severity, rclcpp::Time now) { +bool LogBridgeNode::cooldown_allows(const std::string & fault_code, uint8_t severity, const rclcpp::Time & now) { if (report_cooldown_sec_ <= 0.0) { return true; }