fix(gateway): consistent parameter-config error surface (list vs get) and drop dead classify fallback#543
Merged
Conversation
… classify fallback List configurations now flags itself partial and names unavailable backing nodes instead of silently returning a shrunken 200, matching the 503 the single-parameter GET returns for the same node outage (#541). The dead string-matching fallback in the parameter-error classifier is removed and extracted into a testable unit; a NONE-coded failure now maps to 500, never a 400 misclassification (#542). Closes #541 Closes #542
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the gateway’s parameter-configuration error handling by (1) making GET /{entity}/configurations explicitly surface partial results when some backing ROS 2 nodes are unreachable, and (2) extracting/removing the dead legacy string-matching parameter error classifier in favor of a structured ParameterErrorCode mapping. It also adds unit/handler tests and updates REST API docs to describe the new partial-response metadata.
Changes:
- Surface partial list results via
x-medkit.partial+x-medkit.unavailable_nodesforGET /{entity}/configurationswhen some local backing nodes fail. - Extract structured parameter error classification into
parameter_error_classification.{hpp,cpp}and remove the dead string-matching fallback. - Add new tests to pin error classification and list-vs-get behavior; update REST API documentation accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp | Adds handler-level tests for list endpoint partial signaling and backing-node ordering. |
| src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp | New unit test pinning ParameterErrorCode → HTTP/SOVD error mapping, including NONE regression. |
| src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp | Implements structured error-code classification in a dedicated translation unit. |
| src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp | Uses extracted classifier and adds unavailable_nodes/partial emission in list path. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp | Extends ConfigListXMedkit DTO with unavailable_nodes. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/parameter_error_classification.hpp | Introduces public classifier API + DTO for classification result. |
| src/ros2_medkit_gateway/CMakeLists.txt | Builds new classifier TU and adds the new unit test target. |
| docs/api/rest.rst | Documents the new local-backing-node partial response fields for configuration listing. |
… core Only a genuine 503 availability failure names a backing node "unavailable" and keeps the list partial; a hard shutdown/internal failure now fails the request instead of mislabeling the node behind a partial 200 (matches GET). Move parameter_error_classification.cpp into gateway_core so its neutral core header's symbol comes from the core lib, not the ROS adapter layer.
bburda
reviewed
Jul 19, 2026
…own failure A non-503 backing-node failure no longer fails the whole list and discards the nodes that answered; the list stays 200 partial, keeps their parameters, and names the failed node (503-class in unavailable_nodes, others in failed_nodes). Only when no node answers does the request fail, folding through the severity accumulator so a mixed down+internal outage returns 503 to match GET. Docs, DTO comment and the classification/precedence test comments corrected accordingly.
The /configurations endpoints read a node's ROS 2 parameter service, which returns 503 until that service is up. The lag is small without instrumentation but grows under the TSan job, where the lidar-sensor service was still 503 more than 15s after discovery finished. Tests that did a single GET (or polled with a short timeout) on these endpoints failed intermittently there. Add PARAM_SERVICE_TIMEOUT (90s, generous for the sanitizer slowdown; a longer timeout costs nothing on the passing path since the poll returns as soon as the endpoint answers) and gate every param-service config read on readiness: - test_scenario_config_management, test_configuration_api: setUp waits for the configurations endpoint before each test. - test_scenario_discovery_manifest, test_openapi_response_drift: poll the endpoint instead of asserting 200 on a single early GET.
…st setUp The per-test setUp polled the configurations endpoint up to PARAM_SERVICE_TIMEOUT. Readiness is monotonic, but with the poll in setUp a class of N tests paid the timeout N times, so a genuinely dead service (e.g. 9 tests x 90s) blew the ctest job budget and died on a timeout kill instead of a readable assertion. Cache readiness on the class: only the first test polls; the rest return immediately, and the first timeout marks the class dead so the remaining tests fail fast.
bburda
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two follow-ups from the #540 review, both in the parameter-error path of
config_handlers.cpp.#541 - consistent list vs get error surface
GET /{entity}/configurationssilently dropped a down backing node and returned a plain 200, whileGET /{entity}/configurations/{param}returns 503 for the same outage. The list side now flags the responsepartialand names the down nodes inunavailable_nodes(mirroring the existing peer fan-outpartial/failed_peersconvention) instead of returning a shrunken list with no signal. It stays 200 (partial success is legitimate for a collection); the all-nodes-down case already returns 503. Documented in the REST API guide.#542 - drop dead string-matching classify fallback
Every failure path on the shipped
Ros2ParameterTransportsets a structurederror_code, so theerror_code == NONEstring-matching fallback was dead - and wrong if ever reached (its substrings never matched the real "did not respond" / "not currently set" messages, so failures fell through to 400 invalid-request). The fallback is removed; the classifier is extracted into its own translation unit and a NONE-coded failure now maps to 500 internal-error, never a 400.Tests
test_parameter_error_classificationunit test pins the structured mapping and asserts a NONE failure cannot reach a 400 misclassification.test_parameter_error_precedencegains handler-level cases: one backing node down surfacespartial+unavailable_nodesin both node orders, with a control asserting no partial flag when every node is reachable.Closes #541
Closes #542