From cba6ec7d8a4a5247cf22d591c76d99d680feed47 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Fri, 31 Jul 2026 22:50:54 +0200 Subject: [PATCH 01/17] fix(gateway): dispatch blocking-RPC responses on a shared reentrant callback group server.executor_threads was decorative for RPC-response parallelism: the generic service clients, the per-action client trio, and the per-action status subscriptions all landed in the node default MutuallyExclusive callback group, so one long discovery refresh pass (e.g. a peer aggregation health check) stalled every in-flight service/action response regardless of the configured thread count - the response then surfaced as a bogus service timeout after the full budget (issue #575). Move the blocking-RPC clients (generic service clients + the per-action send_goal/get_result/cancel_goal trio) to one shared Reentrant group and the per-action /_action/status subscriptions to a dedicated MutuallyExclusive group - in-order delivery preserved, decoupled from the default group. Groups are created once at startup by a ros2_common factory (the issue #375 gate bans create_callback_group outside ros2_common/) and injected into the transports as constructor dependencies; the compat shim registers Humble clients into the passed group. Timers and the SSE-fault/trigger-fault/rosout subscriptions stay in the default group by design: refresh passes must remain serialized and those subscriptions rely on in-order delivery. Covered by an executor-starvation falsifier (black-hole aggregation peer keeps a refresh pass permanently in flight; a service-backed operation must still complete fast), a single-thread no-deadlock guard, an executor_threads clamp sweep over the documented [1, 256] range, and a unit pin of the group wiring itself. --- docs/config/server.rst | 27 ++- src/ros2_medkit_gateway/CMakeLists.txt | 7 + .../config/gateway_params.yaml | 13 +- .../compat/generic_client_compat.hpp | 37 ++-- .../ros2_medkit_gateway/gateway_node.hpp | 8 + .../ros2/transports/ros2_action_transport.hpp | 19 +- .../transports/ros2_service_transport.hpp | 11 +- .../ros2_common/callback_groups.hpp | 75 ++++++++ src/ros2_medkit_gateway/src/gateway_node.cpp | 30 +++- src/ros2_medkit_gateway/src/main.cpp | 30 ++-- .../ros2/transports/ros2_action_transport.cpp | 25 ++- .../transports/ros2_service_transport.cpp | 8 +- .../src/ros2_common/callback_groups.cpp | 26 +++ .../test/test_callback_groups.cpp | 102 +++++++++++ .../test/test_generic_client_compat.cpp | 45 +++-- .../test/test_operation_manager.cpp | 7 +- .../test_executor_single_thread.test.py | 110 ++++++++++++ .../features/test_executor_starvation.test.py | 164 ++++++++++++++++++ .../test_thread_pool_starvation.test.py | 96 ++++++++-- 19 files changed, 754 insertions(+), 86 deletions(-) create mode 100644 src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2_common/callback_groups.hpp create mode 100644 src/ros2_medkit_gateway/src/ros2_common/callback_groups.cpp create mode 100644 src/ros2_medkit_gateway/test/test_callback_groups.cpp create mode 100644 src/ros2_medkit_integration_tests/test/features/test_executor_single_thread.test.py create mode 100644 src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py diff --git a/docs/config/server.rst b/docs/config/server.rst index 027442a42..0fe149725 100644 --- a/docs/config/server.rst +++ b/docs/config/server.rst @@ -338,7 +338,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 +364,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..79e663518 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,12 @@ if(BUILD_TESTING) medkit_target_dependencies(test_operation_handlers rclcpp rclcpp_action std_srvs example_interfaces) medkit_set_test_domain(test_operation_handlers) + # 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/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/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/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/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index 03c6ea9f7..f3f46b3fa 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,8 +596,14 @@ 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); + // 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); const auto service_call_timeout_sec = static_cast(declare_parameter("service_call_timeout_sec", static_cast(10))); operation_mgr_ = std::make_unique(service_transport_, action_transport_, discovery_mgr_.get(), 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..dbc43cee0 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()); @@ -440,8 +446,13 @@ 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_; + auto subscription = node_->create_subscription( + status_topic, rclcpp::QoS(10).best_effort(), 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_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_manager.cpp b/src/ros2_medkit_gateway/test/test_operation_manager.cpp index 3f5ee684c..06671cee8 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()); + auto 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); } 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..99e1e4e62 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py @@ -0,0 +1,164 @@ +#!/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 +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. +FAST_COMPLETION_BUDGET_SEC = 8.0 + + +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..a50c8a476 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,25 @@ - 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 unittest +from launch import LaunchDescription import launch_testing +import launch_testing.actions -from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES, get_test_port 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 +51,65 @@ 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 + 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': EXECUTOR_THREADS_CEILING}, + ) + 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 +117,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 +129,31 @@ 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 documented range ceiling (256) is accepted and applied as-is.""" + proc_output.assertWaitFor( + 'Main executor bounded to 256 threads', process=gw_ceiling, timeout=15, + ) + + 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, ) From 2f295230df0d93f367b8cbc4fd722c05f20dfd7f Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Fri, 31 Jul 2026 22:52:20 +0200 Subject: [PATCH 02/17] fix(gateway): report a timed-out action cancel as unknown outcome, not rejection A CancelGoal round-trip that produced no response was collapsed into the same 400 x-medkit-ros2-action-rejected error as a definitive server rejection, and the tracked execution status was never reconciled with the /_action/status stream - the client was told the cancel was refused while the goal was in fact cancelling (issue #576). ActionCancelResult now carries an explicit CancelOutcome set by the transport at every exit, and both cancel entry points (DELETE execution and PUT-stop) share one mapping helper: - timeout: consult the tracked goal fed by the /_action/status stream - if it already shows CANCELING/CANCELED the cancellation is in progress (204, or 202 for PUT-stop); otherwise 504 with the standard SOVD not-responding code and a message pointing at the execution status resource. Tracked status is never hand-written on this path - the status stream stays the authority. - cancel service unavailable: 503 x-medkit-ros2-action-unavailable - transport failure: 500 x-medkit-ros2-action-unavailable - server rejection (return_code 1/2/3): 400 x-medkit-ros2-action-rejected, unchanged The 15s cancel budget floor is removed: cancel uses service_call_timeout_sec like every other action RPC - with timeouts reported honestly and reconciled against the status stream, the floor no longer papers over anything. The execution cancel/stop routes declare their real status codes in OpenAPI and the REST docs, and the stale Common Error Codes table now matches the implemented constants (phantom ERR_TIMEOUT/ERR_INVALID_INPUT/ERR_OPERATION_FAILED/ ERR_INVALID_ENTITY_ID rows replaced, not-responding -> 504 added). Covered by unit-fixture falsifiers driving a raw CancelGoal service that swallows or rejects requests (timeout -> 504 with tracked status untouched, stream-reconciled timeout -> 204/202, rejection -> 400 for both entry points) and an integration test that kills the action server mid-goal and expects 503. The handler-level stop test now asserts the mapped 504 instead of the never-produced ERR_VENDOR_ERROR. --- docs/api/rest.rst | 67 ++- src/ros2_medkit_gateway/CMakeLists.txt | 9 + .../core/http/error_codes.hpp | 6 + .../core/operations/operation_types.hpp | 19 +- .../src/core/managers/operation_manager.cpp | 25 +- .../src/http/handlers/operation_handlers.cpp | 108 ++-- .../src/http/rest_server.cpp | 13 + .../ros2/transports/ros2_action_transport.cpp | 7 + .../test/test_cancel_outcomes.cpp | 475 ++++++++++++++++++ .../test/test_operation_handlers.cpp | 11 +- .../test/test_operation_manager_routing.cpp | 4 + .../test_action_cancel_unavailable.test.py | 196 ++++++++ .../test_scenario_action_lifecycle.test.py | 5 +- 13 files changed, 875 insertions(+), 70 deletions(-) create mode 100644 src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp create mode 100644 src/ros2_medkit_integration_tests/test/features/test_action_cancel_unavailable.test.py diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 663a0875c..a986bbee8 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -697,11 +697,47 @@ Execute Operations ] } +``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 (``invalid-request``) + - **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 + - **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) - **404:** Execution 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``) Lifecycle Endpoints ------------------- @@ -2485,6 +2521,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 +2532,29 @@ 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 + * - ``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/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index 79e663518..31f686a40 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -856,6 +856,15 @@ 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) 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/operations/operation_types.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/operations/operation_types.hpp index e06a32407..9a6752288 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,25 @@ 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). +}; + /// 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/src/core/managers/operation_manager.cpp b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp index bc916e284..c86f3b174 100644 --- a/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp @@ -297,15 +297,6 @@ 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; @@ -327,15 +318,15 @@ ActionCancelResult OperationManager::cancel_action_goal(const std::string & acti 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; 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..ffc1e4629 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -203,6 +203,73 @@ ErrorInfo make_provider_error(const OperationProviderErrorInfo & info, const std return make_plugin_error(info.http_status, info.message, std::move(params)); } +/// 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: +/// - 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). +/// +/// @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; + } + return CancelFailure{504, ERR_NOT_RESPONDING, + std::string(verb) + + " outcome unknown: the action server did not answer the cancel request in time. " + "Poll the execution status resource to observe the goal's progress."}; + } + 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::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 // ============================================================================= @@ -758,24 +825,11 @@ 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 = 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; - } - return tl::make_unexpected(make_error(400, ERR_X_MEDKIT_ROS2_ACTION_REJECTED, error_msg, + return tl::make_unexpected(make_error(failure->http_status, failure->error_code, failure->message, json{{"entity_id", entity_id}, {"operation_id", operation_id}, {"execution_id", execution_id}, @@ -833,7 +887,8 @@ 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) { + auto failure = map_cancel_result(result, *operation_mgr, execution_id, "Stop"); + if (!failure.has_value()) { const std::string base_path = req.path().find("/apps/") != std::string::npos ? "/api/v1/apps/" : "/api/v1/components/"; const std::string location = @@ -847,25 +902,12 @@ OperationHandlers::update_execution(const http::TypedRequest & req, const dto::E 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; - } - return tl::make_unexpected(make_error(400, ERR_X_MEDKIT_ROS2_ACTION_REJECTED, error_msg, + return tl::make_unexpected(make_error(failure->http_status, failure->error_code, failure->message, json{{"entity_id", entity_id}, {"operation_id", operation_id}, {"execution_id", execution_id}, - {"capability", capability}})); + {"capability", capability}, + {"return_code", result.return_code}})); } if (capability == "execute") { return tl::make_unexpected( diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index 958b8b67b..e50bd2037 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -695,6 +695,13 @@ 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 cancel-outcome statuses + // (issue #576) need manual declarations. + .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 +711,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/ros2/transports/ros2_action_transport.cpp b/src/ros2_medkit_gateway/src/ros2/transports/ros2_action_transport.cpp index dbc43cee0..c215057e2 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 @@ -255,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 @@ -283,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; } @@ -300,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; } @@ -321,6 +327,7 @@ 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()); 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..9d1a3f0a6 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp @@ -0,0 +1,475 @@ +// 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 "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; + +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. Two 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 ERROR_REJECTED (return_code=1) at once - +/// the definitive-rejection case. +class PhantomCancelFixtureNode : public rclcpp::Node { + public: + enum class CancelMode { kBlockUntilReleased, kRejectImmediately }; + + 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 = action_msgs::srv::CancelGoal::Response::ERROR_REJECTED; + 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, std::chrono::seconds(30), [this] { + return released_; + }); + response->return_code = action_msgs::srv::CancelGoal::Response::ERROR_NONE; + }); + status_pub_ = + create_publisher("phantom_calibration/_action/status", rclcpp::QoS(10)); + } + + // 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); + } + + 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::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 = 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() + 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() { + raw_req_ = make_request_with_match( + std::string("/api/v1/components/engine/operations/phantom_calibration/executions/") + kGoalIdHex, + R"(/api/v1/components/([^/]+)/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 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"); +} diff --git a/src/ros2_medkit_gateway/test/test_operation_handlers.cpp b/src/ros2_medkit_gateway/test/test_operation_handlers.cpp index 91d5241a3..bdb2c1499 100644 --- a/src/ros2_medkit_gateway/test/test_operation_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_handlers.cpp @@ -588,9 +588,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_routing.cpp b/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp index b199c074d..a818bd7a5 100644 --- a/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp @@ -97,6 +97,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; } @@ -278,6 +279,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()); 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/scenarios/test_scenario_action_lifecycle.test.py b/src/ros2_medkit_integration_tests/test/scenarios/test_scenario_action_lifecycle.test.py index ba4ab5ad8..6610ce1ef 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 @@ -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, From de0af46e9fcdec9e3319b64074d041458dabb849 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sat, 1 Aug 2026 09:54:14 +0200 Subject: [PATCH 03/17] perf(gateway): take the freeze-frame fault event by const reference clang-tidy (performance-unnecessary-value-param) flags the FaultEvent shared_ptr being copied on every subscription callback invocation while only read as const. Pass it by const reference. --- src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) { From ec19f5d557fb95f7e70554a8cbc8237fd8ab5af2 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 16:23:14 +0200 Subject: [PATCH 04/17] fix(log_bridge): read integer parameters as int64 before narrowing declare_parameter() takes the int64 a ROS parameter actually holds and narrows it on the way in, which -Wconversion flags and which lets an out-of-range severity_floor or max_tracked_nodes wrap before its own range check ever runs. Read both as int64, clamp in that domain, and narrow after. --- .../src/log_bridge_node.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) 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..e2cc98e98 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; From 11ccf7b04d0a493dc4df7d0e07e34b16ebc1829b Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 16:27:08 +0200 Subject: [PATCH 05/17] fix(gateway): answer every cancel outcome with what the gateway knows The action status stream is the authority for a goal's state, but the RPC completions were still writing over it. An accepted CancelGoal wrote CANCELING unconditionally, so a stream frame carrying the terminal CANCELED that arrived first was moved backwards permanently: no further frame is ever published, GET kept reporting the goal as running, and it only left the tracking map through the stuck-goal path with a false "action server crashed" warning. update_goal_status now refuses to leave a terminal state - the guard belongs there rather than at the cancel call site because goal-accept and get_result can land late in exactly the same way, while the stream keeps writing directly under the lock. For the same reason a tracked goal must never lose its status stream: cleanup reads the goal count for a path and unsubscribes afterwards, so a goal sent in between kept a subscription that was about to be destroyed, and since the cancel-timeout path reconciles against that stream such a goal could only ever answer 504. unsubscribe_from_action_status now re-checks under the lock that guards the erase and lets a live goal veto it. The manager's three cancel guards never classified themselves and rode the kTransportError default into 500 "action server unavailable" - including for an execution the cleanup timer evicted between the handler's lookup and the manager's re-check, where the truthful answer is the 404 the same request gets a millisecond later. They now carry kNotTracked (404) and kInvalidRequest (400); only the missing-transport guard keeps 500. The mapper is declared in the header so those wire mappings are pinned directly: over HTTP they are reachable only through a race. Also on the response shapes: the reconciled 202 body rendered a hardcoded "running" although the reconcile set includes CANCELED, which an immediate GET of the same execution reports as "failed"; it now renders the tracked status. Its Location header was built from a hardcoded apps/components pair and sent areas and functions clients into the components collection; it now echoes the request path. A timed-out cancel on a goal the gateway already knows to be terminal no longer promises progress that cannot happen, and error parameters carry return_code only when a server actually returned one. The per-return_code wording for codes 1-3 now exists once, in the mapper - the transport's second copy meant deleting a mapper case silently changed the wire message with every test still green. --- .../core/http/handlers/operation_handlers.hpp | 28 ++ .../core/managers/operation_manager.hpp | 17 +- .../core/operations/operation_types.hpp | 8 + .../src/core/managers/operation_manager.cpp | 44 +++- .../src/http/handlers/operation_handlers.cpp | 94 ++++--- .../ros2/transports/ros2_action_transport.cpp | 23 +- .../test/test_cancel_outcomes.cpp | 242 +++++++++++++++++- .../test/test_operation_manager.cpp | 83 +++++- 8 files changed, 478 insertions(+), 61 deletions(-) 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..4465668b4 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 @@ -157,9 +157,19 @@ 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 - a tracked goal without its + /// status stream can never be reconciled by the cancel-timeout path. 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 +193,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 9a6752288..bafed52c4 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 @@ -63,6 +63,14 @@ enum class CancelOutcome : uint8_t { 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, + /// The execution id is malformed - a client error, again with no request + /// ever reaching the action server. + kInvalidRequest, }; /// Result of canceling an action goal. 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 c86f3b174..95a7f93cd 100644 --- a/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp @@ -302,18 +302,26 @@ ActionCancelResult OperationManager::cancel_action_goal(const std::string & acti result.success = false; result.return_code = 0; + // Each guard classifies itself: they all short-circuit before the request + // reaches the action server, so riding the struct's kTransportError default + // would tell the client the action server is unavailable when it is fine. if (!is_valid_uuid_hex(goal_id)) { + result.outcome = CancelOutcome::kInvalidRequest; result.error_message = "Invalid goal_id format: must be 32 hex characters"; return result; } 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; } if (!action_transport_) { + // A missing transport IS a gateway-side transport failure - the only + // guard for which the default classification is the right one. + result.outcome = CancelOutcome::kTransportError; result.error_message = "ActionTransport not configured"; return result; } @@ -393,6 +401,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()) { @@ -404,10 +419,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) { @@ -492,6 +522,14 @@ 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); 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 ffc1e4629..68ea6db01 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -203,14 +203,9 @@ ErrorInfo make_provider_error(const OperationProviderErrorInfo & info, const std return make_plugin_error(info.http_status, info.message, std::move(params)); } -/// 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; -}; +} // namespace + +namespace detail { /// Shared outcome mapping for the two cancel entry points (DELETE execution /// and PUT-stop) - issue #576: @@ -225,7 +220,15 @@ struct CancelFailure { /// - 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). +/// 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. +/// - kInvalidRequest: 400 + `invalid-parameter` - malformed execution id. /// /// @param verb "Cancel" or "Stop" - keeps each entry point's message wording. std::optional map_cancel_result(const ActionCancelResult & result, OperationManager & operation_mgr, @@ -239,10 +242,20 @@ std::optional map_cancel_result(const ActionCancelResult & result (tracked->status == ActionGoalStatus::CANCELING || tracked->status == ActionGoalStatus::CANCELED)) { return std::nullopt; } - return CancelFailure{504, ERR_NOT_RESPONDING, - std::string(verb) + - " outcome unknown: the action server did not answer the cancel request in time. " - "Poll the execution status resource to observe the goal's progress."}; + 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. + if (tracked.has_value() && + (tracked->status == ActionGoalStatus::SUCCEEDED || tracked->status == ActionGoalStatus::ABORTED)) { + message += "The execution status resource already reports the goal as " + + action_status_to_string(tracked->status) + ", so there is nothing left to cancel."; + } else { + message += "Poll the execution status resource to observe the goal's progress."; + } + return CancelFailure{504, ERR_NOT_RESPONDING, std::move(message)}; } case CancelOutcome::kServiceUnavailable: return CancelFailure{503, ERR_X_MEDKIT_ROS2_ACTION_UNAVAILABLE, @@ -250,6 +263,11 @@ std::optional map_cancel_result(const ActionCancelResult & result 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::kInvalidRequest: + return CancelFailure{400, ERR_INVALID_PARAMETER, + result.error_message.empty() ? "Invalid execution id" : result.error_message}; case CancelOutcome::kErrorResponse: break; } @@ -270,7 +288,7 @@ std::optional map_cancel_result(const ActionCancelResult & result return CancelFailure{400, ERR_X_MEDKIT_ROS2_ACTION_REJECTED, std::move(message)}; } -} // namespace +} // namespace detail // ============================================================================= // GET /{entity}/operations - list operations @@ -825,15 +843,19 @@ http::Result OperationHandlers::cancel_execution(const http::Ty } auto result = operation_mgr->cancel_action_goal(goal_info->action_path, execution_id); - auto failure = map_cancel_result(result, *operation_mgr, execution_id, "Cancel"); + auto failure = detail::map_cancel_result(result, *operation_mgr, execution_id, "Cancel"); if (!failure.has_value()) { return http::NoContent{}; } - return tl::make_unexpected(make_error(failure->http_status, failure->error_code, failure->message, - json{{"entity_id", entity_id}, - {"operation_id", operation_id}, - {"execution_id", execution_id}, - {"return_code", result.return_code}})); + 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(failure->http_status, failure->error_code, failure->message, std::move(params))); } // ============================================================================= @@ -887,27 +909,35 @@ 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); - auto failure = map_cancel_result(result, *operation_mgr, execution_id, "Stop"); + auto failure = detail::map_cancel_result(result, *operation_mgr, execution_id, "Stop"); if (!failure.has_value()) { - 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; + // 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()); dto::OperationExecution exec_dto; exec_dto.id = execution_id; - exec_dto.status = "running"; // canceling is still "running" in SOVD terms + // 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. + auto tracked = operation_mgr->get_tracked_goal(execution_id); + exec_dto.status = tracked.has_value() ? sovd_status_from_ros2(tracked->status) : "running"; http::ResponseAttachments att; att.with_status(202).with_header("Location", location); return SuccessPair{std::move(exec_dto), std::move(att)}; } - return tl::make_unexpected(make_error(failure->http_status, failure->error_code, failure->message, - json{{"entity_id", entity_id}, - {"operation_id", operation_id}, - {"execution_id", execution_id}, - {"capability", capability}, - {"return_code", result.return_code}})); + 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(failure->http_status, failure->error_code, failure->message, std::move(params))); } if (capability == "execute") { return tl::make_unexpected( 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 c215057e2..472fb80d0 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 @@ -332,20 +332,15 @@ ActionCancelResult Ros2ActionTransport::cancel_goal(const std::string & action_p 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) { diff --git a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp index 9d1a3f0a6..d7476a5e5 100644 --- a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp +++ b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp @@ -40,6 +40,7 @@ #include #include +#include #include #include #include @@ -112,16 +113,18 @@ httplib::Request make_request_with_match(const std::string & path, const std::st } /// Raw CancelGoal service + status publisher standing in for an action -/// server whose cancel path misbehaves. Two modes: +/// 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 ERROR_REJECTED (return_code=1) at once - -/// the definitive-rejection case. +/// - 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 }; + enum class CancelMode { kBlockUntilReleased, kRejectImmediately, kAcceptImmediately }; PhantomCancelFixtureNode() : rclcpp::Node("phantom_cancel_fixture", "/powertrain/engine") { cancel_service_ = create_service( @@ -129,7 +132,11 @@ class PhantomCancelFixtureNode : public rclcpp::Node { [this](const std::shared_ptr & /*request*/, const std::shared_ptr & response) { if (mode_.load() == CancelMode::kRejectImmediately) { - response->return_code = action_msgs::srv::CancelGoal::Response::ERROR_REJECTED; + 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 @@ -162,6 +169,13 @@ class PhantomCancelFixtureNode : public rclcpp::Node { 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_); @@ -183,6 +197,7 @@ class PhantomCancelFixtureNode : public rclcpp::Node { 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}; @@ -343,9 +358,17 @@ class CancelOutcomesFixtureTest : public ::testing::Test { } http::TypedRequest make_execution_request() { - raw_req_ = make_request_with_match( - std::string("/api/v1/components/engine/operations/phantom_calibration/executions/") + kGoalIdHex, - R"(/api/v1/components/([^/]+)/operations/([^/]+)/executions/([^/]+))"); + 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_); } @@ -473,3 +496,206 @@ TEST_F(CancelOutcomesFixtureTest, PutStopRejectedByServerReturns400Rejected) { 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; + EXPECT_NE(result.error().message.find("succeeded"), std::string::npos) + << "the message must state the terminal status the gateway already knows: " << 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(); +} + +// --------------------------------------------------------------------------- +// 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"); +} + +TEST_F(CancelOutcomesFixtureTest, MalformedExecutionIdMapsTo400InvalidParameter) { + auto * operation_mgr = gateway_node_->get_operation_manager(); + + auto result = operation_mgr->cancel_action_goal(kActionPath, "not-a-uuid"); + auto failure = + ros2_medkit_gateway::handlers::detail::map_cancel_result(result, *operation_mgr, "not-a-uuid", "Cancel"); + + ASSERT_TRUE(failure.has_value()); + EXPECT_EQ(failure->http_status, 400) << failure->error_code << ": " << failure->message; + EXPECT_STREQ(failure->error_code, "invalid-parameter"); +} diff --git a/src/ros2_medkit_gateway/test/test_operation_manager.cpp b/src/ros2_medkit_gateway/test/test_operation_manager.cpp index 06671cee8..6be3a387e 100644 --- a/src/ros2_medkit_gateway/test/test_operation_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_manager.cpp @@ -39,10 +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()); - auto groups = ros2_common::create_gateway_callback_groups(*node_); - service_transport_ = std::make_shared(node_.get(), groups.rpc_reentrant); + 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); + 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); } @@ -55,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_; @@ -423,6 +435,71 @@ 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 ==================== + +// The three manager-side guards never set an outcome, so they ride the +// struct default (kTransportError) into 500 + an availability-flavoured +// error code. The reachable one is the eviction race: the handler finds the +// execution, the cleanup timer evicts it, and the client is told 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"; +} + +TEST_F(TestOperationManager, test_cancel_action_goal_invalid_uuid_is_not_a_transport_error) { + auto result = operation_manager_->cancel_action_goal("/test/action", "invalid_uuid"); + + EXPECT_FALSE(result.success); + EXPECT_NE(result.outcome, CancelOutcome::kTransportError) + << "a malformed execution id is a client error, not an action-transport failure"; +} + +TEST_F(TestOperationManager, test_cancel_action_goal_guards_are_distinguishable) { + auto not_tracked = operation_manager_->cancel_action_goal("/test/action", "00000000000000000000000000000000"); + auto invalid_uuid = operation_manager_->cancel_action_goal("/test/action", "invalid_uuid"); + + EXPECT_NE(not_tracked.outcome, invalid_uuid.outcome) + << "the two guards answer different questions and must not collapse into one wire status"; +} + int main(int argc, char ** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); From 069e001d35f85aa8c5326aec42bf762063cbe49b Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 16:27:08 +0200 Subject: [PATCH 06/17] fix(gateway): bound service_call_timeout_sec and document the cancel budget With the 15s cancel floor gone this parameter is the whole cancel budget, yet it was declared with no range at all and appeared in no document. Zero and negatives were accepted silently and make the response wait expire immediately, so every cancel answers 504 unless the status stream wins the race; an unbounded value pins an HTTP worker for as long as it says. Clamp to [1, 3600] with a warning, like the sibling timeouts, and pin the whole config space - both degenerate directions, both endpoints and the default - against the budget the gateway actually applies rather than the value that was configured. Document the parameter next to its siblings in the server config table and define "cancel budget" where the DELETE/PUT route text uses it: the configured timeout plus up to 2s of cancel-service discovery, which a shorter budget does not shorten. Also correct the Common Error Codes table, where invalid-request was listed as 400-only thirty lines under the PUT block that documents it on 409. --- docs/api/rest.rst | 17 +++++++-- docs/config/server.rst | 15 ++++++++ src/ros2_medkit_gateway/src/gateway_node.cpp | 20 +++++++++-- .../test/test_gateway_node.cpp | 35 +++++++++++++++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index a986bbee8..06bd5f88a 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -739,6 +739,17 @@ Execute Operations 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 ------------------- @@ -2539,8 +2550,10 @@ Vendor-specific ``x-medkit-*`` codes are enveloped: the response carries - 404 - The requested resource (topic, service, parameter) does not exist * - ``invalid-request`` - - 400 - - Invalid request body or missing required parameters + - 400, 409 + - Invalid request body or missing required parameters (400), or a request + that conflicts with the resource's current state - e.g. ``execute`` on + an execution that is still running (409) * - ``invalid-parameter`` - 400 - Invalid parameter value (including malformed entity IDs) diff --git a/docs/config/server.rst b/docs/config/server.rst index 0fe149725..bc6f295af 100644 --- a/docs/config/server.rst +++ b/docs/config/server.rst @@ -166,6 +166,21 @@ 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`` + - Response budget for every operation RPC: ROS 2 service calls + (``POST .../executions`` on a service-backed operation) and all three + action RPCs - send goal, get result, and **cancel**. Values outside + the range are clamped with a warning at startup. Range: 1-3600. + + This is the *cancel budget* referenced by ``DELETE .../executions/{id}`` + and ``PUT .../executions/{id}`` (see :doc:`../api/rest`): a cancel that + gets no answer within it is reported as ``504 not-responding`` unless + the action's status stream already shows the goal cancelling. Discovery + of the cancel service adds up to a further 2 s on top, so with the + minimum of 1 s a cancel issued before the service is discovered can + take up to 3 s before the response wait even starts. .. note:: diff --git a/src/ros2_medkit_gateway/src/gateway_node.cpp b/src/ros2_medkit_gateway/src/gateway_node.cpp index f3f46b3fa..08e230c59 100644 --- a/src/ros2_medkit_gateway/src/gateway_node.cpp +++ b/src/ros2_medkit_gateway/src/gateway_node.cpp @@ -604,8 +604,24 @@ GatewayNode::GatewayNode(const rclcpp::NodeOptions & options) : Node("ros2_medki service_transport_ = std::make_shared(this, callback_groups_.rpc_reentrant); action_transport_ = std::make_shared(this, callback_groups_.rpc_reentrant, callback_groups_.action_status); - const auto service_call_timeout_sec = - static_cast(declare_parameter("service_call_timeout_sec", static_cast(10))); + // 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/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(); From a945127d0ab3c5d04e807791e53fa23065e1e647 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 16:27:08 +0200 Subject: [PATCH 07/17] test(integration): scale in-test budgets under sanitizers, pin the clamped executor The sanitizer jobs multiply every declared CTest timeout, but a budget a test asserts on itself is invisible to that rewrite: an instrumented gateway can blow "must answer within 8s" long before ctest's clock runs out, and the failure reads as a starved response rather than as ASan/TSan overhead. Read the multiplier from MEDKIT_TEST_TIME_SCALE, which those two jobs now set to the same factor they apply to ctest. The unscaled value stays 8s so the falsifier keeps its edge where it was proven red - no scaling turns burning the full 10s service budget and answering 500 into a pass. The executor_threads clamp sweep asserted only startup log lines, so a regression that logged the clamped count and then failed to bring the executor up would have stayed green. Add one real request against the 256-thread gateway. --- .github/workflows/quality.yml | 7 ++++ .../ros2_medkit_test_utils/constants.py | 21 ++++++++++++ .../features/test_executor_starvation.test.py | 11 +++++-- .../test_thread_pool_starvation.test.py | 33 ++++++++++++++++++- 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index a5f0a3b23..aa947d69f 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -342,6 +342,9 @@ 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 run: | source /opt/ros/jazzy/setup.bash source install/setup.bash @@ -445,6 +448,10 @@ 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 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/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..eeea1813b 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,27 @@ 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 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_executor_starvation.test.py b/src/ros2_medkit_integration_tests/test/features/test_executor_starvation.test.py index 99e1e4e62..5e7429e8a 100644 --- 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 @@ -49,7 +49,7 @@ import launch_testing 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 @@ -67,7 +67,14 @@ # 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. -FAST_COMPLETION_BUDGET_SEC = 8.0 +# +# 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(): 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 a50c8a476..b65450cb0 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 @@ -34,13 +34,19 @@ do not depend on request timing. """ +import time import unittest from launch import LaunchDescription import launch_testing import launch_testing.actions +import requests -from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES, get_test_port +from ros2_medkit_test_utils.constants import ( + ALLOWED_EXIT_CODES, + API_BASE_PATH, + get_test_port, +) from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase from ros2_medkit_test_utils.launch_helpers import create_gateway_node @@ -145,6 +151,31 @@ def test_executor_threads_ceiling_applied(self, proc_output, gw_ceiling): 'Main executor bounded to 256 threads', process=gw_ceiling, timeout=15, ) + def test_executor_threads_ceiling_gateway_serves_requests(self): + """The 256-thread 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 + last_error = None + while time.monotonic() < deadline: + try: + response = requests.get(url, timeout=5) + 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=256 gateway never served /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. From 59b2321322e0cddcdf897f54e802c18416b938e0 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 17:47:54 +0200 Subject: [PATCH 08/17] fix(gateway): state the cancel precondition instead of guarding against it Two of the three guards in cancel_action_goal cannot be tripped by a caller that honours the method's contract: both HTTP entry points resolve the execution through get_tracked_goal and answer 404 themselves, every tracked goal_id is produced by uuid_bytes_to_hex, and the action transport is a constructor dependency the gateway always supplies. Giving the malformed-id guard its own wire status was therefore covering an input no caller can build, so drop it and write the precondition down on the declaration instead. The tracked-goal re-check keeps its classification: that one a caller genuinely can trip, by racing the cleanup timer. Pin the surviving consequence end to end as well - after a cancel, the execution must reach a terminal ROS 2 status rather than sitting in "canceling" forever, which is the only place the stream-versus-RPC ordering is visible over HTTP. --- .../core/managers/operation_manager.hpp | 10 +++++++ .../core/operations/operation_types.hpp | 3 -- .../src/core/managers/operation_manager.cpp | 18 +++++++----- .../src/http/handlers/operation_handlers.cpp | 4 --- .../test/test_cancel_outcomes.cpp | 12 -------- .../test/test_operation_manager.cpp | 29 +++++-------------- .../test_scenario_action_lifecycle.test.py | 16 ++++++++++ 7 files changed, 45 insertions(+), 47 deletions(-) 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 4465668b4..86c8b7485 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. 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 bafed52c4..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 @@ -68,9 +68,6 @@ enum class CancelOutcome : uint8_t { /// between the handler's lookup and the manager's own re-check; the /// truthful answer is "no such execution", not "action server unavailable". kNotTracked, - /// The execution id is malformed - a client error, again with no request - /// ever reaching the action server. - kInvalidRequest, }; /// Result of canceling an action goal. 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 95a7f93cd..00b3cb72a 100644 --- a/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp @@ -302,15 +302,19 @@ ActionCancelResult OperationManager::cancel_action_goal(const std::string & acti result.success = false; result.return_code = 0; - // Each guard classifies itself: they all short-circuit before the request - // reaches the action server, so riding the struct's kTransportError default - // would tell the client the action server is unavailable when it is fine. + // 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.outcome = CancelOutcome::kInvalidRequest; 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; @@ -318,10 +322,10 @@ ActionCancelResult OperationManager::cancel_action_goal(const std::string & acti 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_) { - // A missing transport IS a gateway-side transport failure - the only - // guard for which the default classification is the right one. - result.outcome = CancelOutcome::kTransportError; result.error_message = "ActionTransport not configured"; return result; } 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 68ea6db01..66a8bb802 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -228,7 +228,6 @@ namespace detail { /// - 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. -/// - kInvalidRequest: 400 + `invalid-parameter` - malformed execution id. /// /// @param verb "Cancel" or "Stop" - keeps each entry point's message wording. std::optional map_cancel_result(const ActionCancelResult & result, OperationManager & operation_mgr, @@ -265,9 +264,6 @@ std::optional map_cancel_result(const ActionCancelResult & result 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::kInvalidRequest: - return CancelFailure{400, ERR_INVALID_PARAMETER, - result.error_message.empty() ? "Invalid execution id" : result.error_message}; case CancelOutcome::kErrorResponse: break; } diff --git a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp index d7476a5e5..eb61184b7 100644 --- a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp +++ b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp @@ -687,15 +687,3 @@ TEST_F(CancelOutcomesFixtureTest, ExecutionEvictedBetweenChecksMapsTo404NotFound EXPECT_EQ(failure->http_status, 404) << failure->error_code << ": " << failure->message; EXPECT_STREQ(failure->error_code, "resource-not-found"); } - -TEST_F(CancelOutcomesFixtureTest, MalformedExecutionIdMapsTo400InvalidParameter) { - auto * operation_mgr = gateway_node_->get_operation_manager(); - - auto result = operation_mgr->cancel_action_goal(kActionPath, "not-a-uuid"); - auto failure = - ros2_medkit_gateway::handlers::detail::map_cancel_result(result, *operation_mgr, "not-a-uuid", "Cancel"); - - ASSERT_TRUE(failure.has_value()); - EXPECT_EQ(failure->http_status, 400) << failure->error_code << ": " << failure->message; - EXPECT_STREQ(failure->error_code, "invalid-parameter"); -} diff --git a/src/ros2_medkit_gateway/test/test_operation_manager.cpp b/src/ros2_medkit_gateway/test/test_operation_manager.cpp index 6be3a387e..c494902b0 100644 --- a/src/ros2_medkit_gateway/test/test_operation_manager.cpp +++ b/src/ros2_medkit_gateway/test/test_operation_manager.cpp @@ -471,11 +471,14 @@ TEST_F(TestOperationManager, test_unsubscribe_keeps_stream_for_a_goal_that_arriv // ==================== CANCEL GUARD CLASSIFICATION ==================== -// The three manager-side guards never set an outcome, so they ride the -// struct default (kTransportError) into 500 + an availability-flavoured -// error code. The reachable one is the eviction race: the handler finds the -// execution, the cleanup timer evicts it, and the client is told the action -// server is unavailable when the truthful answer is "no such execution". +// 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"); @@ -484,22 +487,6 @@ TEST_F(TestOperationManager, test_cancel_action_goal_not_tracked_is_not_a_transp << "an execution that is not tracked is not an action-transport failure"; } -TEST_F(TestOperationManager, test_cancel_action_goal_invalid_uuid_is_not_a_transport_error) { - auto result = operation_manager_->cancel_action_goal("/test/action", "invalid_uuid"); - - EXPECT_FALSE(result.success); - EXPECT_NE(result.outcome, CancelOutcome::kTransportError) - << "a malformed execution id is a client error, not an action-transport failure"; -} - -TEST_F(TestOperationManager, test_cancel_action_goal_guards_are_distinguishable) { - auto not_tracked = operation_manager_->cancel_action_goal("/test/action", "00000000000000000000000000000000"); - auto invalid_uuid = operation_manager_->cancel_action_goal("/test/action", "invalid_uuid"); - - EXPECT_NE(not_tracked.outcome, invalid_uuid.outcome) - << "the two guards answer different questions and must not collapse into one wire status"; -} - int main(int argc, char ** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); 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 6610ce1ef..0036e4026 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 @@ -156,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, + interval=0.3, + ) + def test_03_service_execution_returns_immediately(self): """Create a Trigger service execution, get immediate result. From 6c9c33e511c1846221269959a123bf628537a807 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 17:53:49 +0200 Subject: [PATCH 09/17] fix(log_bridge): take the cooldown timestamp by const reference rclcpp::Time was copied on every ERROR/FATAL log line only to be read, which clang-tidy flags as performance-unnecessary-value-param. --- .../include/ros2_medkit_log_bridge/log_bridge_node.hpp | 2 +- src/ros2_medkit_log_bridge/src/log_bridge_node.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 e2cc98e98..e66d45292 100644 --- a/src/ros2_medkit_log_bridge/src/log_bridge_node.cpp +++ b/src/ros2_medkit_log_bridge/src/log_bridge_node.cpp @@ -180,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; } From d5397d6b2cb34200f1df1c9329da3930dd6492c2 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 18:07:36 +0200 Subject: [PATCH 10/17] refactor(gateway): bind the stop Location to the request path without copying req.path() already returns a const reference; the reconciled 202 branch was copy-constructing a std::string from it on every accepted stop. --- .../src/http/handlers/operation_handlers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 66a8bb802..7805abf0e 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -911,7 +911,7 @@ OperationHandlers::update_execution(const http::TypedRequest & req, const dto::E // 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()); + const std::string & location = req.path(); dto::OperationExecution exec_dto; exec_dto.id = execution_id; From f7d3fd056cbb05c00e607d09296a1302d2adaa1a Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 4 Aug 2026 18:29:30 +0200 Subject: [PATCH 11/17] fix(gateway): point the created-execution Location at the collection that was posted to The 202 on POST executions built its Location from a hardcoded apps-or-components pair, so on the areas and functions registrations of that route - it is registered for all four entity types - a client was handed a Location in the components collection, naming an entity that is not a component. It also bypassed api_path(). The created execution is a sub-resource of whatever collection the request targeted, so extend the request path instead, matching the bulk-data and lock handlers. Exercising this needs a genuine non-component entity that owns the action, because create_execution rejects a collection/entity-type mismatch before it gets that far, so the handler fixture now also seeds the area its component sits in. --- .../src/http/handlers/operation_handlers.cpp | 9 ++-- .../test/test_operation_handlers.cpp | 46 ++++++++++++++++++- 2 files changed, 51 insertions(+), 4 deletions(-) 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 7805abf0e..ff2a1fe76 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -623,9 +623,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); diff --git a/src/ros2_medkit_gateway/test/test_operation_handlers.cpp b/src/ros2_medkit_gateway/test/test_operation_handlers.cpp index bdb2c1499..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()); From 420b3787a5db678eb4d81594fab98103c3d379fa Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 5 Aug 2026 09:56:19 +0200 Subject: [PATCH 12/17] fix(gateway): restore the status stream for a goal that races the unsubscribe Vetoing the unsubscribe while a goal is tracked only covered the decision, not the transport call that follows it with no lock held. A goal sent into that gap re-flagged the path, got a no-op from the transport because the subscription was still alive at that instant, and then had it destroyed by the unsubscribe already in flight - leaving the goal tracked, the path flagged as subscribed, and no status stream at all. Nothing repaired it, since the flag makes every later subscribe a no-op, so the goal reported "running" until the stuck-goal path evicted it with a warning blaming an action server that had done nothing wrong, and every timed-out cancel for it answered 504 instead of reconciling. Repair the pair after the transport call instead of holding the lock across it: taking our mutex into rclcpp's subscription create/destroy path would add a lock-order edge neither this method nor its subscribe sibling has today. The header now states the postcondition that is actually true and testable - on return, a path with a tracked goal has a live stream. The fake transport gains the idempotence the real one has, so it can no longer silently re-arm itself and hide the window, plus a one-shot re-entry hook that reproduces the production interleaving with no threads. --- .../core/managers/operation_manager.hpp | 11 +++- .../src/core/managers/operation_manager.cpp | 28 +++++++- .../test/test_operation_manager_routing.cpp | 66 +++++++++++++++++++ 3 files changed, 101 insertions(+), 4 deletions(-) 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 86c8b7485..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 @@ -168,8 +168,15 @@ class OperationManager { void subscribe_to_action_status(const std::string & action_path); /// Unsubscribe from action status updates. Idempotent, and a no-op while - /// any goal for the path is still tracked - a tracked goal without its - /// status stream can never be reconciled by the cancel-timeout path. + /// 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 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 00b3cb72a..b6e3a5279 100644 --- a/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp +++ b/src/ros2_medkit_gateway/src/core/managers/operation_manager.cpp @@ -540,9 +540,33 @@ void OperationManager::unsubscribe_from_action_status(const std::string & action 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/test/test_operation_manager_routing.cpp b/src/ros2_medkit_gateway/test/test_operation_manager_routing.cpp index a818bd7a5..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 @@ -119,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); @@ -390,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 From 73f5208196aaa517412a965b63346bd3b4b4efa3 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 5 Aug 2026 09:56:19 +0200 Subject: [PATCH 13/17] fix(gateway): subscribe to action status with the profile the protocol defines The status stream is now the gateway's only source of truth for a goal's terminal state, but it was subscribed KEEP_LAST(10) BEST_EFFORT VOLATILE while rcl_action publishes it KEEP_LAST(1) RELIABLE TRANSIENT_LOCAL. The profiles match, so the bug is not a mismatch - it is durability: the subscription is created only after the goal has been sent, and a VOLATILE reader is delivered nothing on match. An action that finishes inside that window published its terminal frame to a reader that did not exist yet, and no other code path ever re-reads a goal's status, so the execution reported "running" for the rest of its life. Reproduced 2 runs in 3 with an immediate goal. Use rcl_action's own profile, which is what it prescribes for the client side too: the fresh subscription is delivered the writer's last sample on match, which is exactly the frame it missed. KEEP_LAST(1) means the publisher overwrites rather than blocking, so a slow gateway cannot stall an action server; it can coalesce intermediate transitions, which is harmless because only the current status is tracked. The cancel fixture's stand-in publisher offered VOLATILE, which no action server does and which a TRANSIENT_LOCAL reader cannot match - it now makes the same offer as the server it stands in for. --- .../ros2/transports/ros2_action_transport.cpp | 23 ++- .../test/test_cancel_outcomes.cpp | 81 ++++++++++- .../test_action_status_first_goal.test.py | 133 ++++++++++++++++++ 3 files changed, 229 insertions(+), 8 deletions(-) create mode 100644 src/ros2_medkit_integration_tests/test/features/test_action_status_first_goal.test.py 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 472fb80d0..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 @@ -453,8 +453,29 @@ void Ros2ActionTransport::subscribe_status(const std::string & action_path, Stat // 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(10).best_effort(), cb, sub_options); + 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/test/test_cancel_outcomes.cpp b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp index eb61184b7..0d604883b 100644 --- a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp +++ b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp @@ -45,6 +45,8 @@ #include #include #include +#include +#include #include #include #include @@ -74,6 +76,32 @@ 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) { @@ -143,13 +171,19 @@ class PhantomCancelFixtureNode : public rclcpp::Node { // future times out. Bounded and releasable so teardown never hangs // an executor thread. std::unique_lock lock(release_mutex_); - release_cv_.wait_for(lock, std::chrono::seconds(30), [this] { + release_cv_.wait_for(lock, scaled(std::chrono::seconds(30)), [this] { return released_; }); response->return_code = action_msgs::srv::CancelGoal::Response::ERROR_NONE; }); - status_pub_ = - create_publisher("phantom_calibration/_action/status", rclcpp::QoS(10)); + // 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`, @@ -303,7 +337,7 @@ class CancelOutcomesFixtureTest : public ::testing::Test { ASSERT_FALSE(sent.success) << "no send_goal server exists - the priming send must fail"; } - bool wait_for_cancel_service(std::chrono::seconds timeout = std::chrono::seconds(15)) { + 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(); @@ -345,7 +379,7 @@ class CancelOutcomesFixtureTest : public ::testing::Test { 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() + std::chrono::seconds(10); + 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)); @@ -403,6 +437,12 @@ TEST_F(CancelOutcomesFixtureTest, CancelTimeoutReturns504NotRespondingAndLeavesT 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); @@ -597,8 +637,17 @@ TEST_F(CancelOutcomesFixtureTest, CancelTimeoutWithTerminalStatusReturns504Witho 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; - EXPECT_NE(result.error().message.find("succeeded"), std::string::npos) - << "the message must state the terminal status the gateway already knows: " << 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); } @@ -616,6 +665,24 @@ TEST_F(CancelOutcomesFixtureTest, TimeoutErrorParametersCarryNoReturnCode) { << "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 // --------------------------------------------------------------------------- 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..4aaa1b5c3 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_action_status_first_goal.test.py @@ -0,0 +1,133 @@ +#!/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') + + final = self.poll_endpoint_until( + endpoint, + lambda d: d if (d.get('x-medkit') or {}).get('ros2_status') in TERMINAL_ROS2_STATUSES else 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}', + ) From 9778712ee5c6d5089ab939203560b4a732396ed7 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 5 Aug 2026 09:56:46 +0200 Subject: [PATCH 14/17] docs(gateway): make the cancel contract answerable in the words it uses The new 504 told clients to poll the execution status resource, but nothing that resource documents can express a cancel outcome: it renders CANCELED and ABORTED identically as "failed". The advice now names x-medkit.ros2_status, which carries the ROS-level state verbatim, and rest.rst documents that field and corrects the execution example, which named four keys the endpoint does not emit and a status value that is not in its enum. The terminal-status message quoted "succeeded"/"aborted" - raw ROS words the resource it cites never produces. It now quotes that resource's own vocabulary, and its test asserts the agreement rather than the string the handler happens to print. The 409 on re-executing a running execution carried invalid-request, which is not in the SOVD standard code list, while the server already answers its other 409 with precondition-not-fulfilled ("prerequisites not met"). One status, one code; the error table and the OpenAPI declaration follow, the latter having had no 409 at all for a route that returns it. If the goal is evicted between the mapper's read and the response body, the 202 no longer invents "running" for an execution whose own Location answers 404 - it says 404 too. The parameter row claimed one budget for all four operation RPCs. Discovery differs per RPC - bounded by the parameter for send goal, a fixed 2 s for get result and cancel - so it now tabulates what each actually costs. The package README documented cancel as 200 with a JSON body; it returns 204, and none of the outcomes this branch added were there at all. --- docs/api/rest.rst | 54 ++++++++++++++----- docs/config/server.rst | 50 ++++++++++++----- src/ros2_medkit_gateway/README.md | 37 ++++++++++--- .../ros2_medkit_gateway/http/typed_router.hpp | 16 ++++-- .../src/http/handlers/operation_handlers.cpp | 35 +++++++++--- .../src/http/rest_server.cpp | 7 ++- 6 files changed, 152 insertions(+), 47 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 06bd5f88a..6924a69fd 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -688,15 +688,34 @@ 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): @@ -714,7 +733,8 @@ Execute Operations capability is unsupported (``freeze`` / ``reset`` / unknown - ``invalid-parameter``) - **404:** Execution not found - - **409:** ``execute`` on an already-running execution (``invalid-request``) + - **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 @@ -729,8 +749,12 @@ Execute Operations - **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) - - **404:** Execution not found + (``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 @@ -2550,10 +2574,12 @@ Vendor-specific ``x-medkit-*`` codes are enveloped: the response carries - 404 - The requested resource (topic, service, parameter) does not exist * - ``invalid-request`` - - 400, 409 - - Invalid request body or missing required parameters (400), or a request - that conflicts with the resource's current state - e.g. ``execute`` on - an execution that is still running (409) + - 400 + - 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 - Invalid parameter value (including malformed entity IDs) diff --git a/docs/config/server.rst b/docs/config/server.rst index bc6f295af..eb5b864f9 100644 --- a/docs/config/server.rst +++ b/docs/config/server.rst @@ -169,18 +169,44 @@ Data Access Settings * - ``service_call_timeout_sec`` - int - ``10`` - - Response budget for every operation RPC: ROS 2 service calls - (``POST .../executions`` on a service-backed operation) and all three - action RPCs - send goal, get result, and **cancel**. Values outside - the range are clamped with a warning at startup. Range: 1-3600. - - This is the *cancel budget* referenced by ``DELETE .../executions/{id}`` - and ``PUT .../executions/{id}`` (see :doc:`../api/rest`): a cancel that - gets no answer within it is reported as ``504 not-responding`` unless - the action's status stream already shows the goal cancelling. Discovery - of the cancel service adds up to a further 2 s on top, so with the - minimum of 1 s a cancel issued before the service is discovered can - take up to 3 s before the response wait even starts. + - 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:: 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/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/src/http/handlers/operation_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp index ff2a1fe76..c7f9b813e 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -246,13 +246,19 @@ std::optional map_cancel_result(const ActionCancelResult & result // 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. + // 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 " + - action_status_to_string(tracked->status) + ", so there is nothing left to cancel."; + sovd_status_from_ros2(tracked->status) + ", so there is nothing left to cancel."; } else { - message += "Poll the execution status resource to observe the goal's progress."; + // `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)}; } @@ -916,13 +922,25 @@ OperationHandlers::update_execution(const http::TypedRequest & req, const dto::E // areas client into the wrong collection. const std::string & location = req.path(); - dto::OperationExecution exec_dto; - exec_dto.id = execution_id; // 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); - exec_dto.status = tracked.has_value() ? sovd_status_from_ros2(tracked->status) : "running"; + 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 = sovd_status_from_ros2(tracked->status); http::ResponseAttachments att; att.with_status(202).with_header("Location", location); @@ -939,8 +957,11 @@ OperationHandlers::update_execution(const http::TypedRequest & req, const dto::E 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 e50bd2037..fd56e8a9c 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -696,8 +696,11 @@ void RESTServer::setup_routes() { .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 cancel-outcome statuses - // (issue #576) need manual declarations. + // 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)", From adb04a8e1cbd5d7e8a180e4bc4343bcdf741a03f Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 5 Aug 2026 09:56:46 +0200 Subject: [PATCH 15/17] test: scale every wall-clock budget the sanitizer jobs run, not just one MEDKIT_TEST_TIME_SCALE covered a single assertion while three more Python budgets went unscaled and the C++ fixtures had no knob at all - and the sanitizer jobs run the unit suite too. A knob that covers some budgets reads as covering all of them, which is worse than not having one, because a flake in an unscaled budget gets diagnosed as a product regression. Cover the rest: the 256-thread gateway's /health deadline and per-request timeout, the terminal-status polls, and the cancel fixture's service-discovery, status-delivery and teardown budgets. Document the knob in CONTRIBUTING.md, where someone reproducing a sanitizer failure will look for it - it had no mention outside the workflow that sets it. --- CONTRIBUTING.md | 22 +++++++++++++++++++ .../src/http/handlers/operation_handlers.cpp | 9 ++++---- .../test/test_cancel_outcomes.cpp | 5 ++--- .../test_thread_pool_starvation.test.py | 5 +++-- .../test_scenario_action_lifecycle.test.py | 4 ++-- 5 files changed, 33 insertions(+), 12 deletions(-) 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/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp index c7f9b813e..2c244700c 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -257,8 +257,7 @@ std::optional map_cancel_result(const ActionCancelResult & result // `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."; + 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)}; } @@ -933,9 +932,9 @@ OperationHandlers::update_execution(const http::TypedRequest & req, const dto::E // 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}})); + 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; diff --git a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp index 0d604883b..f6cfa4323 100644 --- a/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp +++ b/src/ros2_medkit_gateway/test/test_cancel_outcomes.cpp @@ -45,9 +45,9 @@ #include #include #include +#include #include #include -#include #include #include #include @@ -441,8 +441,7 @@ TEST_F(CancelOutcomesFixtureTest, CancelTimeoutReturns504NotRespondingAndLeavesT // 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 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); 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 b65450cb0..c72e62b27 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 @@ -46,6 +46,7 @@ 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_gateway_node @@ -160,11 +161,11 @@ def test_executor_threads_ceiling_gateway_serves_requests(self): 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 + deadline = time.monotonic() + 30.0 * get_time_scale() last_error = None while time.monotonic() < deadline: try: - response = requests.get(url, timeout=5) + response = requests.get(url, timeout=5 * get_time_scale()) if response.status_code == 200: self.assertEqual(response.json().get('status'), 'healthy') return 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 0036e4026..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 @@ -168,7 +168,7 @@ def test_02_cancel_action_execution(self): 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, + timeout=20.0 * get_time_scale(), interval=0.3, ) From a410c9768a772700a0ece77907f5b045e190421c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 5 Aug 2026 11:11:27 +0200 Subject: [PATCH 16/17] style(integration): wrap the terminal-status poll predicate to the line limit --- .../test/features/test_action_status_first_goal.test.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 index 4aaa1b5c3..1aa012e51 100644 --- 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 @@ -109,9 +109,13 @@ def test_first_goal_that_completes_immediately_reports_terminal_status(self): 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, - lambda d: d if (d.get('x-medkit') or {}).get('ros2_status') in TERMINAL_ROS2_STATUSES else None, + terminal_or_none, timeout=TERMINAL_STATUS_BUDGET_SEC, interval=0.3, ) From 43729a8eb09cbb95863ca5100aaaf851dd09574f Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 5 Aug 2026 15:43:11 +0200 Subject: [PATCH 17/17] test(integration): size the executor ceiling down under sanitizers The TSan job SIGKILLed the sweep's ceiling gateway. The log shows why, and it is not the OOM killer - launch_testing sent the signal itself after its own escalation ran out: "failed to terminate '30' seconds after receiving 'SIGINT', escalating to 'SIGTERM'", then "failed to terminate '45.0' seconds after receiving 'SIGTERM', escalating to 'SIGKILL'". All six functional assertions had already passed, including the /health request, so the gateway started, applied its bound and served correctly; only joining 256 instrumented executor threads outlived the grace period. Unsanitized the same teardown costs nothing measurable - the whole test runs in about 1.3 s - so this is instrumentation cost, not gateway behaviour, and the fix belongs in the harness rather than in the documented range. Resolve the upper endpoint per build: the documented 256 normally, 16 under a sanitizer. Detection reads the sanitizer runtimes' own *SAN_OPTIONS so a future sanitizer job cannot forget to opt in, with MEDKIT_TEST_SANITIZED as an explicit override for reproducing locally; both jobs now set it as well. The constant says plainly that a sanitized run does not pin the documented ceiling and that every normal build still does, so nobody reads coverage into a green sanitizer run that it does not have. The /health request stays, and the pinned "Main executor bounded to %zu threads" wording is untouched. --- .github/workflows/quality.yml | 10 ++++++ .../ros2_medkit_test_utils/constants.py | 23 ++++++++++++ .../test_thread_pool_starvation.test.py | 36 ++++++++++++++++--- 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index aa947d69f..1c85018a5 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -345,6 +345,11 @@ jobs: # 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 @@ -452,6 +457,11 @@ jobs: # 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/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py b/src/ros2_medkit_integration_tests/ros2_medkit_test_utils/constants.py index eeea1813b..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 @@ -43,6 +43,29 @@ def get_time_scale(): 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_thread_pool_starvation.test.py b/src/ros2_medkit_integration_tests/test/features/test_thread_pool_starvation.test.py index c72e62b27..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 @@ -47,6 +47,7 @@ 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_gateway_node @@ -66,6 +67,25 @@ 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(): gateway_node = create_gateway_node( @@ -86,7 +106,7 @@ def generate_test_description(): gw_ceiling = create_gateway_node( port=get_test_port(2), name='gateway_threads_ceiling', - extra_params={'server.executor_threads': EXECUTOR_THREADS_CEILING}, + extra_params={'server.executor_threads': CEILING_UNDER_TEST}, ) gw_invalid = create_gateway_node( port=get_test_port(3), @@ -147,13 +167,18 @@ def test_executor_threads_floor_applied(self, proc_output, gw_floor): ) def test_executor_threads_ceiling_applied(self, proc_output, gw_ceiling): - """The documented range ceiling (256) is accepted and applied as-is.""" + """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( - 'Main executor bounded to 256 threads', process=gw_ceiling, timeout=15, + f'Main executor bounded to {CEILING_UNDER_TEST} threads', + process=gw_ceiling, timeout=15, ) def test_executor_threads_ceiling_gateway_serves_requests(self): - """The 256-thread gateway actually serves, not just logs its bound. + """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 @@ -174,7 +199,8 @@ def test_executor_threads_ceiling_gateway_serves_requests(self): last_error = repr(exc) time.sleep(0.5) self.fail( - f'the executor_threads=256 gateway never served /health: {last_error}' + 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):