fix(gateway): prevent configurations endpoint deadlock on nodes without parameter service

[mfaferek93]

Summary

Prevent gateway HTTP deadlock when configurations endpoint queries nodes without ROS 2 parameter service (e.g., micro-ROS bridges, external nodes).

Root cause: ConfigurationManager used a single param_operations_mutex_ held for the entire parameter operation including blocking wait_for_service() calls. One slow/unavailable node blocked all configuration requests. Self-queries (gateway querying its own parameters) caused permanent deadlock.

Solution:

• Negative cache - after a node parameter service fails to respond, subsequent requests return instantly with SERVICE_UNAVAILABLE for a configurable TTL (default 60s). Hard cap at 500 entries with oldest-entry eviction.
• Self-query guard - gateway uses direct node_->get_parameters() for its own node instead of IPC. Covers all 5 operations (list, get, set, reset, reset_all).
• Narrower spin_mutex_ scope - serializes only ROS 2 IPC calls, not JSON building or cache lookups. Single pre-created param_node_ for fast DDS service discovery.
• Idempotent shutdown with atomic flag + post-shutdown guard on all public methods
• Parameter validation via FloatingPointRange descriptors
• Explicit config_mgr_->shutdown() in GatewayNode destructor

Test plan

• 22 unit tests pass (including new: negative cache, self-query, cross-method cache, concurrent)
• 58 integration tests pass (100%) including test_scenario_discovery_manifest
• Tested on Jetson Orin Nano (aarch64) with ROSMASTER M3 Pro
• Self-query returns 200 in 0.4s (was: permanent deadlock)
• Micro-ROS nodes return instant SERVICE_UNAVAILABLE (was: 4-6s block + gateway freeze)

Fixes #318

[Copilot]

Pull request overview

Improves the gateway’s ROS 2 parameter querying (used by the configurations endpoints) to avoid deadlocks and reduce latency when target nodes don’t provide a parameter service, especially under concurrent requests.

Changes:

• Reworks ConfigurationManager to use per-node mutexes, a negative cache for missing parameter services, and thread-local parameter client nodes.
• Adds a self-query fast path for list/get operations to avoid IPC when querying the gateway’s own node.
• Adds new unit tests intended to cover negative-cache behavior, self-query behavior, and parallelism.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File	Description
src/ros2_medkit_gateway/src/configuration_manager.cpp	Implements negative caching, per-node locks, thread-local clients, self-query logic, and adjusts default timeout.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/configuration_manager.hpp	Updates ConfigurationManager state to support per-node locks and negative cache.
src/ros2_medkit_gateway/test/test_configuration_manager.cpp	Adds new tests for negative cache, self-query behavior, and parallel queries.

Comments suppressed due to low confidence (2)

src/ros2_medkit_gateway/src/configuration_manager.cpp:676

• Same classification issue as reset_parameter(): if cache_default_values() couldn’t run because the parameter service is missing, the first reset_all_parameters() call can return NO_DEFAULTS_CACHED (404) instead of SERVICE_UNAVAILABLE (503). Re-check unavailability after cache_default_values() or have it return a status so the caller can distinguish ‘no defaults cached yet’ from ‘cannot cache because service is unavailable’.

  try {
    cache_default_values(node_name);

    std::vector<rclcpp::Parameter> params_to_reset;
    {
      std::lock_guard<std::mutex> lock(defaults_mutex_);
      auto node_it = default_values_.find(node_name);
      if (node_it == default_values_.end()) {
        result.success = false;
        result.error_message = "No default values cached for node: " + node_name;
        result.error_code = ParameterErrorCode::NO_DEFAULTS_CACHED;
        return result;
      }

src/ros2_medkit_gateway/src/configuration_manager.cpp:601

• If the node has no parameter service, the first reset_parameter() call can return NO_DEFAULTS_CACHED (404) because cache_default_values() returns early after marking the node unavailable, but the function then falls through to the defaults lookup. That misclassifies a service-availability issue as a missing-defaults issue. Consider re-checking is_node_unavailable(node_name) after cache_default_values() (or return a status from cache_default_values) and returning SERVICE_UNAVAILABLE when the cache attempt failed due to missing service.

  try {
    cache_default_values(node_name);

    rclcpp::Parameter default_param;
    {
      std::lock_guard<std::mutex> lock(defaults_mutex_);
      auto node_it = default_values_.find(node_name);
      if (node_it == default_values_.end()) {
        result.success = false;
        result.error_message = "No default values cached for node: " + node_name;
        result.error_code = ParameterErrorCode::NO_DEFAULTS_CACHED;
        return result;

[bburda]

Review: configurations endpoint deadlock fix

The core approach is solid - self-query guard, negative cache, and narrower spin_mutex_ scope are well-designed solutions to a real production problem. The incremental commit history shows thoughtful iteration through different approaches, arriving at the simplest working solution.

A few things to address before merge:

PR description needs update

The description reflects earlier iterations that were tried and reverted during development:

• "Replace single param_operations_mutex_ with per-node mutexes" - the final implementation uses a single spin_mutex_ (same serialization strategy, narrower scope)
• "Thread-local param nodes for SyncParametersClient" - no thread_local in the final code
• "Default parameter_service_timeout_sec reduced from 2.0s to 0.5s" - the default is 2.0s in the final code
• "Concurrent requests to different nodes execute in parallel (per-node mutexes)" - they still serialize through spin_mutex_

GatewayNode shutdown sequence

GatewayNode::~GatewayNode() explicitly calls shutdown() on trigger_mgr_, resource_change_notifier_, plugin_mgr_, etc., but not on config_mgr_. The ConfigurationManager::shutdown() docstring says "Must be called before rclcpp::shutdown()." Since the REST server stops first, the practical risk is low, but for consistency with the other managers it's worth adding an explicit config_mgr_->shutdown() call.

Missing test coverage

The new tests cover the key happy paths, but a few gaps remain:

• Negative cache only tested via list_parameters() - no test verifying cross-method cache hits (e.g., list_parameters marks node unavailable, then get_parameter on the same node returns from cache instantly)
• No test for cache expiry (would need a short-TTL fixture)
• No test for shutdown() lifecycle (idempotent? safe to call methods after?)
• No test for disabled cache (parameter_service_negative_cache_sec: 0)

Inline comments below with specific findings.