gateway: survive a node leaving mid-pass, and keep one lifecycle reader

docs/config/discovery-options.rst

@@ -51,7 +51,8 @@ In runtime mode, the gateway maps the ROS 2 graph to SOVD entities as follows:
 - **Components** - a single host-level Component is created from
   ``HostInfoProvider`` (see Default Component below). No synthetic/heuristic
   Components are created from namespaces.
-- **Apps** - each ROS 2 node becomes an App with ``source: "heuristic"``.
+- **Apps** - each ROS 2 node the graph still attributes an endpoint to becomes
+  an App with ``source: "heuristic"`` (see `What Makes a Node an App`_).
 - **Functions** - namespace grouping creates Function entities (see below).
 
 Default Component
@@ -673,6 +674,73 @@ staleness behavior:
        plugins.parameter_beacon.beacon_ttl_sec: 15.0
        plugins.parameter_beacon.beacon_expiry_sec: 300.0
 
+What Makes a Node an App
+------------------------
+
+Runtime discovery lists the node names on the graph and then asks the graph
+about each name in turn. A name becomes an App only when that second question
+comes back with at least one endpoint: a service, a publisher or a
+subscription. A name the graph attributes nothing to is not turned into an App,
+and where the same App is also declared in a manifest it is linked as
+``x-medkit.is_online: false`` instead.
+
+The rule exists because a name on the graph is not by itself evidence that the
+node is there. Node names and endpoints live in different maps inside the RMW
+graph cache, filled and emptied by different code paths, and the two can
+disagree: a cache can go on naming a node whose endpoints it has already
+removed, and it does not correct itself on a timer, because the removal event
+for a participant is generated once. A gateway that trusted the name alone
+would keep serving that App for as long as the process runs. Asking about the
+endpoints costs nothing extra in the normal case - discovery already reads each
+node's services to build its operations - and it answers the question the name
+cannot.
+
+The same answer covers the ordinary race. Anything may happen between listing
+the names and asking about one of them, including the node exiting; rcl then
+reports the name as non-existent and raises. That is read the same way: the
+node is not part of this pass, the pass finishes normally, and the next pass
+decides again from a fresh read.
+
+The boundary, stated as a limit rather than as a promise: a node that
+advertises no service, no publisher and no subscription at all is not visible as
+an App.
+
+What an rclcpp node puts on the graph, and what a ``NodeOptions`` flag can take
+away:
+
+.. list-table::
+   :header-rows: 1
+   :widths: 40 25 35
+
+   * - Entity
+     - Distro
+     - Switched off by
+   * - the six parameter services
+     - all
+     - ``start_parameter_services(false)``
+   * - ``/parameter_events`` publisher
+     - all
+     - ``start_parameter_event_publisher(false)``
+   * - ``/rosout`` publisher
+     - all
+     - ``enable_rosout(false)``
+   * - ``/parameter_events`` subscription (the node's time source watches
+       ``use_sim_time``)
+     - all
+     - nothing - no ``NodeOptions`` flag reaches it
+   * - ``~/get_type_description`` service
+     - Jazzy and newer
+     - the read-only ``start_type_description_service`` parameter
+
+So a node that switches off everything ``NodeOptions`` offers is still visible:
+its time-source subscription alone keeps it an App on every supported distro.
+Reaching the boundary takes a node built below rclcpp - an rcl-level node with
+no endpoints of any kind - or an rclcpp node whose time source has been taken
+away. The fixture ``demo_silent_node`` in ``ros2_medkit_integration_tests``
+carries exactly one endpoint of its own, the ``/rosout`` publisher, and its test
+asserts that the graph still attributes it to the node and that the gateway
+still lists it.
+
 See Also
 --------
 

docs/tutorials/heuristic-apps.rst

@@ -113,7 +113,8 @@ Entity Model
 
 In runtime mode, the gateway maps the ROS 2 graph as follows:
 
-- **Apps** - each ROS 2 node becomes an App (``source: "heuristic"``)
+- **Apps** - each ROS 2 node the graph still attributes an endpoint to becomes
+  an App (``source: "heuristic"``); see :doc:`/config/discovery-options`
 - **Functions** - namespace grouping creates Function entities
 - **Components** - a single host-level Component from ``HostInfoProvider``
 - **Areas** - not created (Areas come from manifest only)

src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/src/param_beacon_plugin.cpp

@@ -121,6 +121,16 @@ void ParameterBeaconPlugin::set_context(PluginContext & context) {
   options.start_parameter_event_publisher(false);
   options.use_global_arguments(false);
   param_node_ = std::make_shared<rclcpp::Node>("_param_beacon_node", options);
+  // Registered with the context's GraphListener here, while the context is
+  // known valid. The beacon's parameter sweeps wait for services, and a
+  // shutdown landing on this node's first such wait would leave it marked as
+  // registered while absent from the listener's list:
+  // NodeGraph::get_graph_event() spends should_add_to_graph_listener_ before
+  // add_node() throws GraphListenerShutdownError, and ~NodeGraph then throws
+  // NodeNotFoundError out of a noexcept destructor. The window is narrowed, not
+  // closed: a shutdown between the make_shared above and this line spends the
+  // flag the same way, and rclcpp offers no way to un-spend it.
+  (void)param_node_->get_graph_event();
 
   // Set default client factory if not injected (tests inject mock factory)
   if (!client_factory_) {

src/ros2_medkit_gateway/CMakeLists.txt

@@ -882,6 +882,11 @@ if(BUILD_TESTING)
   target_link_libraries(test_ros2_lifecycle_state_reader gateway_ros2)
   medkit_target_dependencies(test_ros2_lifecycle_state_reader rclcpp lifecycle_msgs)
 
+  # Helper nodes that outlive rclcpp::shutdown() (each test cycles the context)
+  medkit_add_gtest(test_graph_listener_shutdown_safety test/test_graph_listener_shutdown_safety.cpp)
+  target_link_libraries(test_graph_listener_shutdown_safety gateway_ros2)
+  medkit_target_dependencies(test_graph_listener_shutdown_safety rclcpp lifecycle_msgs)
+
   # Add operation handler tests
   medkit_add_gtest(test_operation_handlers test/test_operation_handlers.cpp)
   target_link_libraries(test_operation_handlers gateway_ros2)

src/ros2_medkit_gateway/README.md

@@ -1790,7 +1790,9 @@ In addition to standard ROS 2 node discovery, the gateway supports **topic-based
 In runtime discovery mode, the gateway maps the ROS 2 graph to the SOVD entity model:
 
 - **Component**: A single host-derived Component is created from `HostInfoProvider` (hostname, OS, architecture). All Apps belong to this Component.
-- **App**: Each discovered ROS 2 node becomes an App entity.
+- **App**: Each discovered ROS 2 node becomes an App entity, as long as the graph
+  still attributes at least one service, publisher or subscription to it - a name
+  with no endpoints left is a node that has gone, not an App.
 - **Function**: The first namespace segment creates a Function entity that groups all Apps under that namespace (e.g., `/powertrain/engine/temp_sensor` and `/powertrain/engine/rpm_sensor` both belong to Function `powertrain`).
 - **Area**: Areas are only created from manifest definitions. They are never auto-generated in runtime mode. Use hybrid or manifest-only mode to organize entities into Areas.
 

src/ros2_medkit_gateway/design/lifecycle.rst

@@ -94,10 +94,22 @@ plain node with no lifecycle services, the status falls back to ``App::is_online
 the ROS 2 graph), which is the best signal available for an unmanaged node. A node that is not
 online short-circuits to ``"notReady"`` without a GetState read (an offline node cannot be
 ``active``), which also avoids a blocking read against a crashed managed node whose services still
-linger in the cache. The GetState read runs on a private node and executor (spun inline), so it
-never blocks or races the gateway executor; it is, however, serialized by an internal mutex, so a
-reachable-but-slow managed node holds that mutex across its spin and delays other concurrent
-``/status`` reads for up to the (short) read timeout.
+linger in the cache. The GetState read runs on a private node, and on an executor created for that one call and spun
+inline, so it never blocks or races the gateway executor. That private node is named after the
+gateway, so there is exactly one reader per gateway:
+``GatewayNode::get_lifecycle_state_reader()`` creates it on first use and both the ``/status``
+handler and any plugin that reads lifecycle state (through
+``RosPluginContext::lifecycle_state_reader()``) share it.
+
+Sharing one object between an HTTP handler and a plugin tick is only safe because the reader's
+mutex covers just the two things rclcpp does not make thread-safe - creating and destroying the
+call's callback group and client on the shared node. The service wait, the request and the spin
+run outside it, so a read of one node never waits for a read of another. That matters because the
+graph watchdog's lifecycle watcher seeds nodes that may never answer: with the mutex spanning the
+spin, a ``/status`` read of a healthy node was measured queueing behind one such seed for the full
+read timeout, against single-digit milliseconds once the mutex was narrowed
+(``test_lifecycle_reader_contention_e2e``). Destruction is not ordered by that mutex either - a
+call spends most of its life outside it - but by an in-flight count the destructor waits on.
 
 **Component status:** the synthetic host component (the one carrying ``host_metadata``,
 populated by ``HostInfoProvider``) is ``"ready"`` while the gateway is serving the request -

src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp

@@ -17,6 +17,7 @@
 #include <atomic>
 #include <chrono>
 #include <memory>
+#include <mutex>
 #include <rclcpp/rclcpp.hpp>
 #include <string>
 #include <thread>
@@ -50,6 +51,7 @@
 #include "ros2_medkit_gateway/core/plugins/plugin_manager.hpp"
 #include "ros2_medkit_gateway/core/resource_change_notifier.hpp"
 #include "ros2_medkit_gateway/core/resource_sampler.hpp"
+#include "ros2_medkit_gateway/core/status/lifecycle_state_reader.hpp"
 #include "ros2_medkit_gateway/core/subscription_transport.hpp"
 #include "ros2_medkit_gateway/core/trigger_store.hpp"
 #include "ros2_medkit_gateway/discovery/discovery_manager.hpp"
@@ -196,6 +198,18 @@ class GatewayNode : public rclcpp::Node {
    */
   EntityFreezeFrameCapture * get_entity_freeze_frame_capture() const;
 
+  /**
+   * @brief The gateway's lifecycle-state reader, created on first use.
+   *
+   * One instance per gateway. The reader owns a private ROS node named after
+   * this one, so a second instance would put a second node of that exact name
+   * on the graph: DDS warns about the collision, and every graph query that
+   * lists nodes then returns the name twice.
+   *
+   * @return Shared pointer; never null.
+   */
+  std::shared_ptr<LifecycleStateReader> get_lifecycle_state_reader();
+
   /**
    * @brief Route the trigger topic subscriber through the shared subscription
    *        executor (issue #548). Its per-trigger subscriptions are then
@@ -418,6 +432,11 @@ class GatewayNode : public rclcpp::Node {
   std::unique_ptr<TriggerFaultSubscriber> trigger_fault_subscriber_;
   // Zero-config freeze-frames for plugin-backed entities (nullptr when disabled)
   std::unique_ptr<EntityFreezeFrameCapture> entity_freeze_frame_capture_;
+  // Shared by the /status handler and by any plugin that reads lifecycle state,
+  // guarded because the REST server and the plugin manager reach it from
+  // different threads during start-up.
+  std::shared_ptr<LifecycleStateReader> lifecycle_state_reader_;
+  std::mutex lifecycle_state_reader_mutex_;
   // Config-less threshold-rule engine + its dedicated evaluation loop (issue
   // #235). The thread is joined in ~GatewayNode BEFORE plugin/fault shutdown.
   std::unique_ptr<FaultTriggerEngine> fault_trigger_engine_;

src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/ros_plugin_context.hpp

@@ -17,6 +17,7 @@
 #include <memory>
 
 #include "ros2_medkit_gateway/core/plugins/plugin_context.hpp"
+#include "ros2_medkit_gateway/core/status/lifecycle_state_reader.hpp"
 
 namespace rclcpp {
 class Node;
@@ -43,6 +44,17 @@ class RosPluginContext : public PluginContext {
  public:
   /// Get the ROS 2 node pointer for subscriptions, service clients, etc.
   virtual rclcpp::Node * node() const = 0;
+
+  /// The gateway's own lifecycle-state reader, for plugins that read managed
+  /// nodes' states. Shared on purpose: the reader owns a private ROS node named
+  /// after the gateway, so a plugin that built its own would put a second node
+  /// of that exact name on the graph.
+  ///
+  /// Returns nullptr for contexts that are not backed by a gateway (test
+  /// doubles); a plugin that gets nullptr owns the fallback.
+  virtual std::shared_ptr<LifecycleStateReader> lifecycle_state_reader() const {
+    return nullptr;
+  }
 };
 
 /**

src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/providers/ros2_runtime_introspection.hpp

@@ -96,6 +96,24 @@ class Ros2RuntimeIntrospection : public IntrospectionProvider {
   /// call from hot paths.
   std::vector<App> discover_apps();
 
+  /// Services the graph attributes to the node `name` in namespace `ns`,
+  /// including the internal parameter services, or nullopt when the graph
+  /// attributes no service, publisher or subscription to it at all.
+  ///
+  /// Nullopt is the "this name is not a node any more" answer, and it covers
+  /// both shapes a departure takes. rcl answers the per-node query for a name
+  /// it no longer knows with RCL_RET_NODE_NAME_NON_EXISTENT, which rclcpp
+  /// raises; and an rmw graph cache can keep a node name after the node's
+  /// endpoints have already been removed from the same cache, in which case
+  /// every per-node query comes back empty instead. Both mean the node is not
+  /// there, so both read the same way here.
+  ///
+  /// The publisher and subscription queries only run when the node reports no
+  /// services, so a node with the default parameter services costs exactly one
+  /// query, as before.
+  std::optional<std::map<std::string, std::vector<std::string>>> services_of_present_node(const std::string & name,
+                                                                                          const std::string & ns) const;
+
   /// Group nodes by namespace into Function entities (no graph query).
   std::vector<Function> discover_functions(const std::vector<App> & apps);
 

src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2/status/ros2_lifecycle_state_reader.hpp

@@ -15,6 +15,7 @@
 #pragma once
 
 #include <chrono>
+#include <condition_variable>
 #include <memory>
 #include <mutex>
 #include <optional>
@@ -27,17 +28,24 @@
 namespace ros2_medkit_gateway {
 
 /// LifecycleStateReader backed by lifecycle_msgs/srv/GetState. The GetState client
-/// runs on a private node driven by a private SingleThreadedExecutor that is spun
-/// inline on the calling thread (no background spin), so it never races the host
-/// gateway node's MultiThreadedExecutor (the private-node/private-executor idea is
-/// borrowed from ros2_fault_service_transport.cpp; unlike that transport, the target
-/// service path varies per app, so the client is created per call rather than once
-/// in the constructor). create_client, async_send_request, the inline spin, and the
-/// client teardown are serialized by an internal mutex; wait_for_service runs outside
-/// it (backed by an independent graph listener) so an unreachable node does not hold
-/// the mutex. A reachable-but-slow node still holds the mutex across its spin for up to
-/// the timeout and serializes other concurrent /status reads, so the default timeout is
-/// kept short.
+/// runs on a private node, never on the host gateway node, so it cannot race that
+/// node's MultiThreadedExecutor (the private-node idea is borrowed from
+/// ros2_fault_service_transport.cpp; unlike that transport, the target service path
+/// varies per app, so the client is created per call rather than once in the
+/// constructor).
+///
+/// One instance serves every caller in the process - the /status handler and any
+/// plugin that reads lifecycle state - so a slow or unanswering target must not be
+/// able to delay a caller asking about a different node. Each call therefore gets its
+/// own callback group and its own executor, spun inline on the calling thread, and the
+/// internal mutex covers only what rclcpp does not make thread-safe: creating and
+/// destroying that group and client on the shared node. The service wait, the request
+/// and the spin all run outside it, so concurrent reads overlap.
+///
+/// The mutex is not what makes destruction safe, since a call spends most of its life
+/// outside it. `in_flight_` is: the destructor refuses new calls and waits for the
+/// ones already running, so the private node outlives every executor that references
+/// it.
 class Ros2LifecycleStateReader : public LifecycleStateReader {
  public:
   explicit Ros2LifecycleStateReader(rclcpp::Node * host,
@@ -52,9 +60,15 @@ class Ros2LifecycleStateReader : public LifecycleStateReader {
 
  private:
   std::shared_ptr<rclcpp::Node> client_node_;
-  std::shared_ptr<rclcpp::executors::SingleThreadedExecutor> executor_;
   std::chrono::duration<double> timeout_;
+  /// Guards client_node_'s callback-group and client registries, which rclcpp
+  /// does not serialize, plus the two fields below.
   std::mutex mutex_;
+  std::condition_variable idle_cv_;
+  /// Calls that have created their client and not yet destroyed it.
+  int in_flight_{0};
+  /// Set by the destructor; a call that sees it returns without touching the node.
+  bool stopping_{false};
 };
 
 }  // namespace ros2_medkit_gateway

src/ros2_medkit_gateway/include/ros2_medkit_gateway/ros2_common/callback_groups.hpp

@@ -72,4 +72,22 @@ struct GatewayCallbackGroups {
  */
 GatewayCallbackGroups create_gateway_callback_groups(rclcpp::Node & node);
 
+/**
+ * @brief A MutuallyExclusive group on @p node that no executor collects along
+ * with the node.
+ *
+ * For an entity created, used and destroyed inside a single call on a single
+ * thread. `automatically_add_to_executor_with_node = false` means the group is
+ * reachable only from the executor that call builds for it, so no other thread
+ * ever holds a reference to the entity and the entity can be destroyed on the
+ * calling thread rather than on an executor thread - the ordering rule the
+ * gateway relies on everywhere it creates ROS entities while running.
+ *
+ * Unlike `create_gateway_callback_groups`, this is called while the node is
+ * live, so the caller owns the serialisation: every call must be made under the
+ * same lock as every other node-mutating call on @p node. Group creation is one
+ * of the rcl hash-map mutations the issue-#375 gate exists for.
+ */
+rclcpp::CallbackGroup::SharedPtr create_isolated_callback_group(rclcpp::Node & node);
+
 }  // namespace ros2_medkit_gateway::ros2_common

src/ros2_medkit_gateway/src/gateway_node.cpp

@@ -38,6 +38,7 @@
 #include "ros2_medkit_gateway/core/thread_pool_config.hpp"
 #include "ros2_medkit_gateway/param_utils.hpp"
 #include "ros2_medkit_gateway/plugins/ros_plugin_context.hpp"
+#include "ros2_medkit_gateway/ros2/status/ros2_lifecycle_state_reader.hpp"
 
 #include "ros2_medkit_gateway/core/http/handlers/sse_transport_provider.hpp"
 #include "ros2_medkit_gateway/core/sqlite_trigger_store.hpp"
@@ -1915,6 +1916,14 @@ EntityFreezeFrameCapture * GatewayNode::get_entity_freeze_frame_capture() const
   return entity_freeze_frame_capture_.get();
 }
 
+std::shared_ptr<LifecycleStateReader> GatewayNode::get_lifecycle_state_reader() {
+  std::lock_guard<std::mutex> lock(lifecycle_state_reader_mutex_);
+  if (!lifecycle_state_reader_) {
+    lifecycle_state_reader_ = std::make_shared<Ros2LifecycleStateReader>(this);
+  }
+  return lifecycle_state_reader_;
+}
+
 void GatewayNode::set_trigger_subscription_executor(ros2_common::Ros2SubscriptionExecutor & exec) {
   if (trigger_topic_subscriber_) {
     trigger_topic_subscriber_->set_subscription_executor(&exec);