From a10f2d87cae38409ff0d40195e7c95f9d1032c55 Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Thu, 16 Jul 2026 22:00:26 +0200 Subject: [PATCH 1/3] fix(gateway): non-404 parameter errors win over NOT_FOUND across nodes The multi-node GET probe overwrote last_result on every failure, so a NOT_FOUND from a later node masked a timeout/unavailable from an earlier one and the client got 404 instead of 503. Fold failures through ParameterErrorAccumulator so the surfaced error is order independent. --- src/ros2_medkit_gateway/CMakeLists.txt | 5 ++ .../core/http/handlers/config_handlers.hpp | 38 +++++++++ .../src/http/handlers/config_handlers.cpp | 47 ++++++----- .../test/test_parameter_error_precedence.cpp | 84 +++++++++++++++++++ 4 files changed, 152 insertions(+), 22 deletions(-) create mode 100644 src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index d4cad2a65..d1da57c05 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -592,6 +592,10 @@ if(BUILD_TESTING) ament_add_gtest(test_error_info test/test_error_info.cpp) target_link_libraries(test_error_info gateway_ros2) + # Parameter error precedence tests (multi-node GET aggregation) + ament_add_gtest(test_parameter_error_precedence test/test_parameter_error_precedence.cpp) + target_link_libraries(test_parameter_error_precedence gateway_ros2) + # Add ROS 2 subscription executor tests ament_add_gtest(test_ros2_subscription_executor test/test_ros2_subscription_executor.cpp) target_link_libraries(test_ros2_subscription_executor gateway_ros2) @@ -1051,6 +1055,7 @@ if(BUILD_TESTING) test_tls_config test_fault_manager test_error_info + test_parameter_error_precedence test_lifecycle_status_helpers test_ros2_lifecycle_state_reader test_ros2_subscription_executor diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp index e258fa7ce..52bc85765 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp @@ -14,8 +14,10 @@ #pragma once +#include #include +#include "ros2_medkit_gateway/core/configuration/parameter_types.hpp" #include "ros2_medkit_gateway/dto/config.hpp" #include "ros2_medkit_gateway/http/handlers/handler_context.hpp" #include "ros2_medkit_gateway/http/response_types.hpp" @@ -24,6 +26,42 @@ namespace ros2_medkit_gateway { namespace handlers { +/// HTTP status + SOVD error code for a failed parameter operation. +struct ParameterErrorClassification { + int status_code = 500; + std::string error_code; +}; + +/// Folds per-node parameter lookup failures so the error surfaced when no +/// node succeeds does not depend on node iteration order: a non-404 failure +/// (node unavailable, timeout, ...) always wins over NOT_FOUND, and a 404 +/// never replaces a held non-404. +class ParameterErrorAccumulator { + public: + /// Fold one failed ParameterResult. + void add(const ParameterResult & result); + + /// True when every folded failure classified as 404. + bool all_not_found() const { + return all_not_found_; + } + + /// The failure to surface: the latest non-404 folded, else the latest 404. + const ParameterResult & worst() const { + return worst_; + } + + /// Classification of worst(). + const ParameterErrorClassification & classification() const { + return classification_; + } + + private: + ParameterResult worst_; + ParameterErrorClassification classification_; + bool all_not_found_ = true; +}; + /** * @brief Handlers for configuration (parameter) REST API endpoints. * diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index cd4534d36..fe0fcf44f 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -114,16 +114,10 @@ const NodeConfigInfo * find_node_for_app(const std::vector & nod return nullptr; } -/// Error classification result for parameter operations. -struct ErrorClassification { - int status_code; - std::string error_code; -}; - /// Map a structured `ParameterErrorCode` to an HTTP status + SOVD error code. /// Identical mapping to the legacy `classify_error_code` helper. -ErrorClassification classify_error_code(ParameterErrorCode error_code) { - ErrorClassification result; +ParameterErrorClassification classify_error_code(ParameterErrorCode error_code) { + ParameterErrorClassification result; switch (error_code) { case ParameterErrorCode::NOT_FOUND: @@ -165,14 +159,14 @@ ErrorClassification classify_error_code(ParameterErrorCode error_code) { /// Classify a ParameterResult into HTTP status + error code. Prefers the /// structured `error_code` field and falls back to string-matching the legacy /// error messages for backward compatibility with older transports. -ErrorClassification classify_parameter_error(const ParameterResult & result) { +ParameterErrorClassification classify_parameter_error(const ParameterResult & result) { // Prefer structured error code if (result.error_code != ParameterErrorCode::NONE) { return classify_error_code(result.error_code); } // Fallback to string parsing for backward compatibility - ErrorClassification classification; + ParameterErrorClassification classification; const auto & error_message = result.error_message; if (error_message.find("not found") != std::string::npos || @@ -314,6 +308,19 @@ void apply_fan_out_observability(dto::ConfigListXMedkit & xm, } // namespace +void ParameterErrorAccumulator::add(const ParameterResult & result) { + auto err = classify_parameter_error(result); + const bool not_found = err.status_code == 404; + // Non-404 failures win; a 404 never replaces a held non-404. + if (!not_found || all_not_found_) { + worst_ = result; + classification_ = std::move(err); + } + if (!not_found) { + all_not_found_ = false; + } +} + // =========================================================================== // GET /{entity-path}/configurations // =========================================================================== @@ -476,10 +483,10 @@ http::Result ConfigHandlers::get_configuration(cons // For non-aggregated entities (or aggregated entities without a prefix) we // probe every backing node for the parameter and return the first success. - // Track errors so a non-404 from any node (e.g. unavailable) wins over a - // pure "not found" verdict in the no-success branch. - ParameterResult last_result; - bool all_not_found = true; + // Failures fold through ParameterErrorAccumulator so a non-404 from any + // node (e.g. unavailable) wins over a pure "not found" verdict regardless + // of node iteration order. + ParameterErrorAccumulator errors; for (const auto & node_info : agg_configs.nodes) { auto result = config_mgr->get_parameter(node_info.node_fqn, parsed.param_name); @@ -491,23 +498,19 @@ http::Result ConfigHandlers::get_configuration(cons return make_read_value(entity_id, node_info.node_fqn, parsed.param_name, source_app, result.data); } - last_result = result; - auto err = classify_parameter_error(result); - if (err.status_code != 404) { - all_not_found = false; - } + errors.add(result); } - if (all_not_found) { + if (errors.all_not_found()) { return tl::unexpected(make_error(404, ERR_RESOURCE_NOT_FOUND, "Parameter not found", json{{"entity_id", entity_id}, {"id", param_id}})); } // Some node reported a non-404 (e.g. unavailable); surface that. - auto err = classify_parameter_error(last_result); + const auto & err = errors.classification(); return tl::unexpected( make_error(err.status_code, err.error_code, "Failed to get parameter from any node", - json{{"details", last_result.error_message}, {"entity_id", entity_id}, {"id", param_id}})); + json{{"details", errors.worst().error_message}, {"entity_id", entity_id}, {"id", param_id}})); } // =========================================================================== diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp new file mode 100644 index 000000000..f0b8b4072 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -0,0 +1,84 @@ +// 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 + +#include + +#include "ros2_medkit_gateway/core/configuration/parameter_types.hpp" +#include "ros2_medkit_gateway/core/http/error_codes.hpp" +#include "ros2_medkit_gateway/core/http/handlers/config_handlers.hpp" + +namespace ros2_medkit_gateway { +namespace { + +using handlers::ParameterErrorAccumulator; + +ParameterResult make_failure(ParameterErrorCode code, const std::string & message) { + ParameterResult r; + r.success = false; + r.error_message = message; + r.error_code = code; + return r; +} + +// Regression for the multi-node GET probe loop: a NOT_FOUND from one node +// must not mask a timeout/unavailable from another, in either iteration +// order. + +TEST(ParameterErrorPrecedence, TimeoutThenNotFoundSurfacesUnavailable) { + ParameterErrorAccumulator acc; + acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + + EXPECT_FALSE(acc.all_not_found()); + EXPECT_EQ(acc.classification().status_code, 503); + EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); +} + +TEST(ParameterErrorPrecedence, NotFoundThenTimeoutSurfacesUnavailable) { + ParameterErrorAccumulator acc; + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); + + EXPECT_FALSE(acc.all_not_found()); + EXPECT_EQ(acc.classification().status_code, 503); + EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); +} + +TEST(ParameterErrorPrecedence, NotFoundNeverReplacesUnavailable) { + ParameterErrorAccumulator acc; + acc.add(make_failure(ParameterErrorCode::SERVICE_UNAVAILABLE, "service not available")); + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + + EXPECT_FALSE(acc.all_not_found()); + EXPECT_EQ(acc.classification().status_code, 503); + EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::SERVICE_UNAVAILABLE); +} + +TEST(ParameterErrorPrecedence, AllNotFoundStays404) { + ParameterErrorAccumulator acc; + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + + EXPECT_TRUE(acc.all_not_found()); + EXPECT_EQ(acc.classification().status_code, 404); + EXPECT_EQ(acc.classification().error_code, ERR_RESOURCE_NOT_FOUND); +} + +} // namespace +} // namespace ros2_medkit_gateway From 591578ddcd424233c23c6ec0b83b0a484f87f349 Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Fri, 17 Jul 2026 10:51:00 +0200 Subject: [PATCH 2/3] fix(gateway): rank multi-node parameter errors by severity Among distinct non-404 failures the last one folded won, so a partially-broken aggregated entity surfaced 503 or 500 depending on node iteration order. Rank 503 > other 5xx > non-404 4xx > 404 and keep the first failure at the winning rank. --- .../core/http/handlers/config_handlers.hpp | 10 +++-- .../src/http/handlers/config_handlers.cpp | 36 +++++++++++---- .../test/test_parameter_error_precedence.cpp | 44 +++++++++++++++++++ 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp index 52bc85765..78c9cc786 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp @@ -33,9 +33,10 @@ struct ParameterErrorClassification { }; /// Folds per-node parameter lookup failures so the error surfaced when no -/// node succeeds does not depend on node iteration order: a non-404 failure -/// (node unavailable, timeout, ...) always wins over NOT_FOUND, and a 404 -/// never replaces a held non-404. +/// node succeeds does not depend on node iteration order. Failures are +/// ranked by severity: node unavailable/timeout (503) > other server errors +/// (5xx) > non-404 client errors (4xx) > NOT_FOUND (404). The highest rank +/// folded so far is kept; within a rank the first failure folded wins. class ParameterErrorAccumulator { public: /// Fold one failed ParameterResult. @@ -46,7 +47,7 @@ class ParameterErrorAccumulator { return all_not_found_; } - /// The failure to surface: the latest non-404 folded, else the latest 404. + /// The failure to surface: the highest-severity failure folded so far. const ParameterResult & worst() const { return worst_; } @@ -59,6 +60,7 @@ class ParameterErrorAccumulator { private: ParameterResult worst_; ParameterErrorClassification classification_; + int worst_rank_ = -1; bool all_not_found_ = true; }; diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index fe0fcf44f..ac33a6f94 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -306,18 +306,36 @@ void apply_fan_out_observability(dto::ConfigListXMedkit & xm, } } +/// Severity rank for order-independent folding: node-unavailable (503) is the +/// most actionable verdict (retry), then other server errors, then non-404 +/// client errors; NOT_FOUND ranks lowest so it can never mask a real failure. +int severity_rank(int status_code) { + if (status_code == 404) { + return 0; + } + if (status_code == 503) { + return 3; + } + if (status_code >= 500) { + return 2; + } + return 1; // non-404 4xx +} + } // namespace void ParameterErrorAccumulator::add(const ParameterResult & result) { auto err = classify_parameter_error(result); - const bool not_found = err.status_code == 404; - // Non-404 failures win; a 404 never replaces a held non-404. - if (!not_found || all_not_found_) { + const int rank = severity_rank(err.status_code); + if (rank > 0) { + all_not_found_ = false; + } + // Strictly-greater keeps the first failure folded at the winning rank, so + // the surfaced status never depends on node iteration order. + if (rank > worst_rank_) { worst_ = result; classification_ = std::move(err); - } - if (!not_found) { - all_not_found_ = false; + worst_rank_ = rank; } } @@ -483,9 +501,9 @@ http::Result ConfigHandlers::get_configuration(cons // For non-aggregated entities (or aggregated entities without a prefix) we // probe every backing node for the parameter and return the first success. - // Failures fold through ParameterErrorAccumulator so a non-404 from any - // node (e.g. unavailable) wins over a pure "not found" verdict regardless - // of node iteration order. + // Failures fold through ParameterErrorAccumulator, which ranks them by + // severity (503 > other 5xx > non-404 4xx > 404), so the surfaced error is + // the same regardless of node iteration order. ParameterErrorAccumulator errors; for (const auto & node_info : agg_configs.nodes) { diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp index f0b8b4072..92cf7e1c7 100644 --- a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -70,6 +70,50 @@ TEST(ParameterErrorPrecedence, NotFoundNeverReplacesUnavailable) { EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::SERVICE_UNAVAILABLE); } +// Three distinct verdicts folded in both orders: the surfaced error must be +// the highest-severity one (503), not whichever non-404 happened to fold last. + +TEST(ParameterErrorPrecedence, ThreeNodesInternalThenTimeoutThenNotFoundSurfacesUnavailable) { + ParameterErrorAccumulator acc; + acc.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); + acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + + EXPECT_FALSE(acc.all_not_found()); + EXPECT_EQ(acc.classification().status_code, 503); + EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); +} + +TEST(ParameterErrorPrecedence, ThreeNodesNotFoundThenTimeoutThenInternalSurfacesUnavailable) { + ParameterErrorAccumulator acc; + acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); + acc.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); + + EXPECT_FALSE(acc.all_not_found()); + EXPECT_EQ(acc.classification().status_code, 503); + EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); +} + +TEST(ParameterErrorPrecedence, ServerErrorOutranksClientErrorBothOrders) { + ParameterErrorAccumulator first_order; + first_order.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); + first_order.add(make_failure(ParameterErrorCode::INVALID_VALUE, "bad value")); + + ParameterErrorAccumulator second_order; + second_order.add(make_failure(ParameterErrorCode::INVALID_VALUE, "bad value")); + second_order.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); + + for (const auto * acc : {&first_order, &second_order}) { + EXPECT_FALSE(acc->all_not_found()); + EXPECT_EQ(acc->classification().status_code, 500); + EXPECT_EQ(acc->classification().error_code, ERR_INTERNAL_ERROR); + EXPECT_EQ(acc->worst().error_code, ParameterErrorCode::INTERNAL_ERROR); + } +} + TEST(ParameterErrorPrecedence, AllNotFoundStays404) { ParameterErrorAccumulator acc; acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); From 4328f81fea689b9df69cb971684133c6103387f6 Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Fri, 17 Jul 2026 12:52:25 +0200 Subject: [PATCH 3/3] test(gateway): prove parameter error precedence at handler level Replace the accumulator-only unit tests with ConfigHandlers::get_configuration tests against a live GatewayNode (unavailable, unresponsive and reachable backing nodes, both iteration orders) and move ParameterErrorAccumulator back into the .cpp anonymous namespace so no test-only surface leaks from the header. --- src/ros2_medkit_gateway/CMakeLists.txt | 4 +- .../core/http/handlers/config_handlers.hpp | 40 --- .../src/http/handlers/config_handlers.cpp | 61 +++- .../test/test_parameter_error_precedence.cpp | 309 +++++++++++++----- 4 files changed, 278 insertions(+), 136 deletions(-) diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index d1da57c05..04386729e 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -592,9 +592,11 @@ if(BUILD_TESTING) ament_add_gtest(test_error_info test/test_error_info.cpp) target_link_libraries(test_error_info gateway_ros2) - # Parameter error precedence tests (multi-node GET aggregation) + # Parameter error precedence tests (multi-node GET aggregation, live GatewayNode) ament_add_gtest(test_parameter_error_precedence test/test_parameter_error_precedence.cpp) target_link_libraries(test_parameter_error_precedence gateway_ros2) + medkit_target_dependencies(test_parameter_error_precedence rclcpp) + medkit_set_test_domain(test_parameter_error_precedence) # Add ROS 2 subscription executor tests ament_add_gtest(test_ros2_subscription_executor test/test_ros2_subscription_executor.cpp) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp index 78c9cc786..e258fa7ce 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/config_handlers.hpp @@ -14,10 +14,8 @@ #pragma once -#include #include -#include "ros2_medkit_gateway/core/configuration/parameter_types.hpp" #include "ros2_medkit_gateway/dto/config.hpp" #include "ros2_medkit_gateway/http/handlers/handler_context.hpp" #include "ros2_medkit_gateway/http/response_types.hpp" @@ -26,44 +24,6 @@ namespace ros2_medkit_gateway { namespace handlers { -/// HTTP status + SOVD error code for a failed parameter operation. -struct ParameterErrorClassification { - int status_code = 500; - std::string error_code; -}; - -/// Folds per-node parameter lookup failures so the error surfaced when no -/// node succeeds does not depend on node iteration order. Failures are -/// ranked by severity: node unavailable/timeout (503) > other server errors -/// (5xx) > non-404 client errors (4xx) > NOT_FOUND (404). The highest rank -/// folded so far is kept; within a rank the first failure folded wins. -class ParameterErrorAccumulator { - public: - /// Fold one failed ParameterResult. - void add(const ParameterResult & result); - - /// True when every folded failure classified as 404. - bool all_not_found() const { - return all_not_found_; - } - - /// The failure to surface: the highest-severity failure folded so far. - const ParameterResult & worst() const { - return worst_; - } - - /// Classification of worst(). - const ParameterErrorClassification & classification() const { - return classification_; - } - - private: - ParameterResult worst_; - ParameterErrorClassification classification_; - int worst_rank_ = -1; - bool all_not_found_ = true; -}; - /** * @brief Handlers for configuration (parameter) REST API endpoints. * diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index ac33a6f94..097874400 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -114,6 +114,12 @@ const NodeConfigInfo * find_node_for_app(const std::vector & nod return nullptr; } +/// HTTP status + SOVD error code for a failed parameter operation. +struct ParameterErrorClassification { + int status_code = 500; + std::string error_code; +}; + /// Map a structured `ParameterErrorCode` to an HTTP status + SOVD error code. /// Identical mapping to the legacy `classify_error_code` helper. ParameterErrorClassification classify_error_code(ParameterErrorCode error_code) { @@ -322,22 +328,51 @@ int severity_rank(int status_code) { return 1; // non-404 4xx } -} // namespace +/// Folds per-node parameter lookup failures so the error surfaced when no +/// node succeeds does not depend on node iteration order. Failures are +/// ranked by `severity_rank`; the highest rank folded so far is kept, and +/// within a rank the first failure folded wins. +class ParameterErrorAccumulator { + public: + /// Fold one failed ParameterResult. + void add(const ParameterResult & result) { + auto err = classify_parameter_error(result); + const int rank = severity_rank(err.status_code); + if (rank > 0) { + all_not_found_ = false; + } + // Strictly-greater keeps the first failure folded at the winning rank, so + // the surfaced status never depends on node iteration order. + if (rank > worst_rank_) { + worst_ = result; + classification_ = std::move(err); + worst_rank_ = rank; + } + } -void ParameterErrorAccumulator::add(const ParameterResult & result) { - auto err = classify_parameter_error(result); - const int rank = severity_rank(err.status_code); - if (rank > 0) { - all_not_found_ = false; + /// True when every folded failure classified as 404. + bool all_not_found() const { + return all_not_found_; } - // Strictly-greater keeps the first failure folded at the winning rank, so - // the surfaced status never depends on node iteration order. - if (rank > worst_rank_) { - worst_ = result; - classification_ = std::move(err); - worst_rank_ = rank; + + /// The failure to surface: the highest-severity failure folded so far. + const ParameterResult & worst() const { + return worst_; } -} + + /// Classification of worst(). + const ParameterErrorClassification & classification() const { + return classification_; + } + + private: + ParameterResult worst_; + ParameterErrorClassification classification_; + int worst_rank_ = -1; + bool all_not_found_ = true; +}; + +} // namespace // =========================================================================== // GET /{entity-path}/configurations diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp index 92cf7e1c7..ddb164984 100644 --- a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -14,115 +14,260 @@ #include +#include +#include + +#include +#include +#include #include +#include +#include -#include "ros2_medkit_gateway/core/configuration/parameter_types.hpp" +#include "ros2_medkit_gateway/core/discovery/models/app.hpp" +#include "ros2_medkit_gateway/core/discovery/models/component.hpp" #include "ros2_medkit_gateway/core/http/error_codes.hpp" #include "ros2_medkit_gateway/core/http/handlers/config_handlers.hpp" +#include "ros2_medkit_gateway/core/models/thread_safe_entity_cache.hpp" +#include "ros2_medkit_gateway/gateway_node.hpp" +#include "ros2_medkit_gateway/http/typed_router.hpp" + +using ros2_medkit_gateway::App; +using ros2_medkit_gateway::AuthConfig; +using ros2_medkit_gateway::Component; +using ros2_medkit_gateway::CorsConfig; +using ros2_medkit_gateway::ERR_RESOURCE_NOT_FOUND; +using ros2_medkit_gateway::ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; +using ros2_medkit_gateway::ErrorInfo; +using ros2_medkit_gateway::GatewayNode; +using ros2_medkit_gateway::ThreadSafeEntityCache; +using ros2_medkit_gateway::TlsConfig; +using ros2_medkit_gateway::handlers::ConfigHandlers; +using ros2_medkit_gateway::handlers::HandlerContext; +namespace http = ros2_medkit_gateway::http; -namespace ros2_medkit_gateway { namespace { -using handlers::ParameterErrorAccumulator; +constexpr const char * kMissingParam = "definitely_missing_param"; +constexpr const char * kGetConfigPattern = R"(/api/v1/components/([^/]+)/configurations/([^/]+))"; -ParameterResult make_failure(ParameterErrorCode code, const std::string & message) { - ParameterResult r; - r.success = false; - r.error_message = message; - r.error_code = code; - return r; +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; } -// Regression for the multi-node GET probe loop: a NOT_FOUND from one node -// must not mask a timeout/unavailable from another, in either iteration -// order. +} // namespace -TEST(ParameterErrorPrecedence, TimeoutThenNotFoundSurfacesUnavailable) { - ParameterErrorAccumulator acc; - acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); +// ============================================================================= +// Handler-level regression tests for the multi-node probe loop in +// GET /{entity}/configurations/{param}: when no backing node succeeds, the +// surfaced error must be the highest-severity per-node failure regardless of +// node iteration order (a NOT_FOUND from one node must never mask an +// unavailable/timeout from another). Exercised end-to-end through +// ConfigHandlers::get_configuration against a live GatewayNode with real +// per-node failures: +// - "ghost" node FQN with no ROS node behind it -> SERVICE_UNAVAILABLE +// - unspun node (parameter services exist, never answered) -> TIMEOUT +// - the gateway's own node missing the parameter -> NOT_FOUND +// ============================================================================= - EXPECT_FALSE(acc.all_not_found()); - EXPECT_EQ(acc.classification().status_code, 503); - EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); - EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); -} +class ParameterErrorPrecedenceTest : public ::testing::Test { + protected: + static void SetUpTestSuite() { + rclcpp::init(0, nullptr); + } -TEST(ParameterErrorPrecedence, NotFoundThenTimeoutSurfacesUnavailable) { - ParameterErrorAccumulator acc; - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); - acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); + static void TearDownTestSuite() { + if (rclcpp::ok()) { + rclcpp::shutdown(); + } + } - EXPECT_FALSE(acc.all_not_found()); - EXPECT_EQ(acc.classification().status_code, 503); - EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); - EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); -} + void SetUp() override { + rclcpp::NodeOptions options; + options.parameter_overrides({ + // Fail fast on the ghost node's wait_for_service and keep the test + // runtime bounded. + rclcpp::Parameter("parameter_service_timeout_sec", 1.0), + // Disable the negative cache so every probe classifies fresh + // (TIMEOUT stays TIMEOUT instead of a cached SERVICE_UNAVAILABLE). + rclcpp::Parameter("parameter_service_negative_cache_sec", 0.0), + }); + gateway_node_ = std::make_shared(options); + ASSERT_NE(gateway_node_, nullptr); + // The cache is injected below; a graph-driven refresh would wipe it. + gateway_node_->stop_discovery_refresh_for_testing(); -TEST(ParameterErrorPrecedence, NotFoundNeverReplacesUnavailable) { - ParameterErrorAccumulator acc; - acc.add(make_failure(ParameterErrorCode::SERVICE_UNAVAILABLE, "service not available")); - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + // A node whose parameter services exist in the DDS graph but are never + // spun: parameter requests to it time out instead of failing fast. + unspun_node_ = std::make_shared("param_precedence_unspun"); - EXPECT_FALSE(acc.all_not_found()); - EXPECT_EQ(acc.classification().status_code, 503); - EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::SERVICE_UNAVAILABLE); -} + executor_ = std::make_unique(); + executor_->add_node(gateway_node_); + spin_thread_ = std::thread([this]() { + executor_->spin(); + }); -// Three distinct verdicts folded in both orders: the surfaced error must be -// the highest-severity one (503), not whichever non-404 happened to fold last. + ctx_ = std::make_unique(gateway_node_.get(), cors_, auth_, tls_, nullptr); + handlers_ = std::make_unique(*ctx_); -TEST(ParameterErrorPrecedence, ThreeNodesInternalThenTimeoutThenNotFoundSurfacesUnavailable) { - ParameterErrorAccumulator acc; - acc.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); - acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); + wait_for_unspun_node_discovery(); + seed_cache(); + } - EXPECT_FALSE(acc.all_not_found()); - EXPECT_EQ(acc.classification().status_code, 503); - EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); - EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); -} + void TearDown() override { + if (executor_) { + executor_->cancel(); + } + if (spin_thread_.joinable()) { + spin_thread_.join(); + } + handlers_.reset(); + ctx_.reset(); + executor_.reset(); + unspun_node_.reset(); + gateway_node_.reset(); + } -TEST(ParameterErrorPrecedence, ThreeNodesNotFoundThenTimeoutThenInternalSurfacesUnavailable) { - ParameterErrorAccumulator acc; - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); - acc.add(make_failure(ParameterErrorCode::TIMEOUT, "timed out")); - acc.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); + /// Wait until the gateway sees the unspun node's parameter services in the + /// graph, so its probes deterministically reach the TIMEOUT path (service + /// discovered, request never answered) instead of SERVICE_UNAVAILABLE. + void wait_for_unspun_node_discovery() { + const std::string service = unspun_fqn() + "/get_parameters"; + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (std::chrono::steady_clock::now() < deadline) { + auto services = gateway_node_->get_service_names_and_types(); + if (services.count(service) > 0) { + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + FAIL() << "unspun node parameter service not discovered: " << service; + } - EXPECT_FALSE(acc.all_not_found()); - EXPECT_EQ(acc.classification().status_code, 503); - EXPECT_EQ(acc.classification().error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); - EXPECT_EQ(acc.worst().error_code, ParameterErrorCode::TIMEOUT); -} + std::string self_fqn() const { + return gateway_node_->get_fully_qualified_name(); + } + + std::string unspun_fqn() const { + return unspun_node_->get_fully_qualified_name(); + } + + static App make_app(const std::string & id, const std::string & component_id, const std::string & fqn) { + App app; + app.id = id; + app.name = id; + app.component_id = component_id; + app.bound_fqn = fqn; + app.is_online = true; + return app; + } + + /// Seed aggregated components whose backing-node order is fixed by app + /// insertion order (get_component_configurations preserves it), covering + /// both iteration orders for every scenario. + void seed_cache() { + const std::string ghost_fqn = "/param_precedence_ghost"; -TEST(ParameterErrorPrecedence, ServerErrorOutranksClientErrorBothOrders) { - ParameterErrorAccumulator first_order; - first_order.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); - first_order.add(make_failure(ParameterErrorCode::INVALID_VALUE, "bad value")); + std::vector components; + std::vector apps; - ParameterErrorAccumulator second_order; - second_order.add(make_failure(ParameterErrorCode::INVALID_VALUE, "bad value")); - second_order.add(make_failure(ParameterErrorCode::INTERNAL_ERROR, "node crashed")); + auto add_component = [&](const std::string & id, const std::vector & fqns) { + Component comp; + comp.id = id; + comp.name = id; + components.push_back(comp); + for (size_t i = 0; i < fqns.size(); ++i) { + apps.push_back(make_app(id + "_app" + std::to_string(i), id, fqns[i])); + } + }; - for (const auto * acc : {&first_order, &second_order}) { - EXPECT_FALSE(acc->all_not_found()); - EXPECT_EQ(acc->classification().status_code, 500); - EXPECT_EQ(acc->classification().error_code, ERR_INTERNAL_ERROR); - EXPECT_EQ(acc->worst().error_code, ParameterErrorCode::INTERNAL_ERROR); + // Unavailable node first / last, plus a node that lacks the parameter. + add_component("stack_unavail_first", {ghost_fqn, self_fqn()}); + add_component("stack_unavail_last", {self_fqn(), ghost_fqn}); + + // Three distinct verdicts (TIMEOUT, SERVICE_UNAVAILABLE, NOT_FOUND) in + // both orders. + add_component("stack_mixed_timeout_first", {unspun_fqn(), ghost_fqn, self_fqn()}); + add_component("stack_mixed_timeout_last", {self_fqn(), ghost_fqn, unspun_fqn()}); + + // Every node reachable but none has the parameter. + add_component("stack_all_missing", {self_fqn(), self_fqn()}); + + auto & cache = const_cast(gateway_node_->get_thread_safe_cache()); + cache.update_all({}, components, apps, {}); } + + ErrorInfo get_missing_param_error(const std::string & component_id) { + const std::string path = "/api/v1/components/" + component_id + "/configurations/" + kMissingParam; + auto raw = make_request_with_match(path, kGetConfigPattern); + http::TypedRequest req(raw); + + auto result = handlers_->get_configuration(req); + EXPECT_FALSE(result.has_value()) << "expected an error for " << path; + return result.has_value() ? ErrorInfo{} : result.error(); + } + + CorsConfig cors_{}; + AuthConfig auth_{}; + TlsConfig tls_{}; + std::shared_ptr gateway_node_; + std::shared_ptr unspun_node_; + std::unique_ptr executor_; + std::thread spin_thread_; + std::unique_ptr ctx_; + std::unique_ptr handlers_; +}; + +// Regression order for the pre-fix overwrite bug: the unavailable node is +// probed first, then the reachable node reports NOT_FOUND last. The old code +// classified only the LAST failure, so this surfaced 404 instead of 503. +TEST_F(ParameterErrorPrecedenceTest, UnavailableNodeFirstNotFoundLastReturns503) { + auto err = get_missing_param_error("stack_unavail_first"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); } -TEST(ParameterErrorPrecedence, AllNotFoundStays404) { - ParameterErrorAccumulator acc; - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); - acc.add(make_failure(ParameterErrorCode::NOT_FOUND, "Parameter not found")); +// Opposite iteration order must surface the same verdict. +TEST_F(ParameterErrorPrecedenceTest, NotFoundFirstUnavailableNodeLastReturns503) { + auto err = get_missing_param_error("stack_unavail_last"); - EXPECT_TRUE(acc.all_not_found()); - EXPECT_EQ(acc.classification().status_code, 404); - EXPECT_EQ(acc.classification().error_code, ERR_RESOURCE_NOT_FOUND); + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); } -} // namespace -} // namespace ros2_medkit_gateway +// Three distinct per-node verdicts (TIMEOUT, SERVICE_UNAVAILABLE, NOT_FOUND): +// a 503-class failure must win over NOT_FOUND, and within the 503 rank the +// first failure probed is the one surfaced in details. +TEST_F(ParameterErrorPrecedenceTest, MixedThreeNodeFailuresTimeoutFirstReturns503) { + auto err = get_missing_param_error("stack_mixed_timeout_first"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + // First 503-rank failure probed: the unspun node's TIMEOUT. + ASSERT_TRUE(err.params.contains("details")); + EXPECT_NE(err.params["details"].get().find("did not respond"), std::string::npos) << err.params.dump(); +} + +TEST_F(ParameterErrorPrecedenceTest, MixedThreeNodeFailuresTimeoutLastReturns503) { + auto err = get_missing_param_error("stack_mixed_timeout_last"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + // First 503-rank failure probed: the ghost node's SERVICE_UNAVAILABLE. + ASSERT_TRUE(err.params.contains("details")); + EXPECT_NE(err.params["details"].get().find("not available"), std::string::npos) << err.params.dump(); +} + +// When every backing node is reachable and reports NOT_FOUND, the aggregate +// verdict stays a plain 404. +TEST_F(ParameterErrorPrecedenceTest, AllNodesMissingParameterReturns404) { + auto err = get_missing_param_error("stack_all_missing"); + + EXPECT_EQ(err.http_status, 404); + EXPECT_EQ(err.code, ERR_RESOURCE_NOT_FOUND); +}