Detect a node that dies when nothing else is left to report it

src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/aggregated_fault.hpp

@@ -29,8 +29,8 @@ namespace ros2_medkit_graph_watchdog {
 /// external entity from its own IntrospectionProvider (graph_watchdog_plugin.cpp).
 ///
 /// It must be an entity the plugin owns, not a borrowed one. Scoping these faults to the
-/// host Component - the obvious choice, and what this used to do - makes them reachable
-/// from NO endpoint: `collect_component_app_fqns` (fault_scope.cpp) only puts a
+/// host Component instead - the obvious-looking alternative - makes them reachable from NO
+/// endpoint: `collect_component_app_fqns` (fault_scope.cpp) only puts a
 /// component's bare id in the scope set when `external` is true, and a runtime host
 /// Component built by HostInfoProvider never sets it, so `/components/<host>/faults` and
 /// the scoped detail route both drop the fault. There is no server-level
@@ -72,31 +72,35 @@ class AggregatedFault {
   AggregatedFault(const char * code, std::uint8_t severity) : code_(code), severity_(severity) {
   }
 
-  void emit(DetectorContext & ctx, const std::map<std::string, std::string> & affected) const {
+  /// Returns whatever ctx.raise_fault()/ctx.clear_fault() returned - true only if the
+  /// request actually reached async_send_request(), never merely because `affected` was
+  /// non-empty. That proves local enqueue, not fault_manager receipt (the client is
+  /// fire-and-forget) - see DetectorContext::raise_fault's own doc for exactly what this
+  /// return value does and does not prove.
+  bool emit(DetectorContext & ctx, const std::map<std::string, std::string> & affected) const {
     const std::string source = graph_source_id(ctx);
     if (affected.empty()) {
-      ctx.clear_fault(code_, source);
-      return;
+      return ctx.clear_fault(code_, source);
     }
-    ctx.raise_fault(code_, severity_, describe(affected), source);
+    return ctx.raise_fault(code_, severity_, describe(affected), source);
   }
 
-  /// emit() with a caller-chosen ORDER for the description.
+  /// emit() with a caller-chosen ORDER for the description. Return value: see emit()'s own
+  /// doc.
   ///
   /// The map overload above lists entities lexicographically, which is fine when every
   /// entry is equally interesting. It is not fine when the set mixes long-standing entries
   /// with a brand-new one and the text is capped: the new entry can sort last and be cut.
   /// `order` names the keys of `affected` in the order they should appear; any key of
   /// `affected` missing from `order` is appended afterwards, so a caller can never silently
   /// drop an entity by getting the ordering wrong.
-  void emit_ordered(DetectorContext & ctx, const std::map<std::string, std::string> & affected,
+  bool emit_ordered(DetectorContext & ctx, const std::map<std::string, std::string> & affected,
                     const std::vector<std::string> & order) const {
     const std::string source = graph_source_id(ctx);
     if (affected.empty()) {
-      ctx.clear_fault(code_, source);
-      return;
+      return ctx.clear_fault(code_, source);
     }
-    ctx.raise_fault(code_, severity_, describe_ordered(affected, order), source);
+    return ctx.raise_fault(code_, severity_, describe_ordered(affected, order), source);
   }
 
   /// Join the affected entities into one description, capped at kMaxDescriptionChars.

src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/allowlist_suppressor.hpp

@@ -0,0 +1,94 @@
+// 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 <set>
+#include <string>
+#include <utility>
+
+#include "ros2_medkit_graph_watchdog/suppressor.hpp"
+
+namespace ros2_medkit_graph_watchdog {
+
+/// Operator-declared veto. Suppresses a key iff it - or a name the SAME entity is also
+/// known by - is present, verbatim, in the configured allow set: never a prefix or a
+/// substring, so allowlisting `/r1/x` can never reach into `/r2/x` on a fleet where the two
+/// share a suffix.
+///
+/// Deliberately mirrors lifecycle_expectation_detector.cpp's own require_active matching
+/// (`id != app.id && id != fqn && id != leaf`) rather than inventing a second convention:
+/// a bare name is the natural operator input, and `App::id` is unstable - it gains a
+/// namespace prefix the moment a same-bare-name collision exists anywhere in the graph -
+/// so the two config surfaces (require_active, allowlist) need to accept the same shapes or
+/// an operator who has one working would reasonably expect the other to behave the same way
+/// and be wrong.
+///
+/// The two surfaces reach that same three-way match through different mechanics, though:
+/// lifecycle_expectation matches while the node is still PRESENT, so `app.id` and its fqn
+/// are both in hand from the very same `ctx.snapshot->apps` entry it is iterating. This
+/// class is asked about a key ONLY after the entity is already gone - present-tense
+/// `ctx.snapshot` cannot answer "what was this dead key's id" at all. suppresses() itself
+/// therefore still only ever sees a single string and covers the two forms derivable from
+/// that string alone (the key verbatim, and its own bare leaf); the id form needs the
+/// caller to have captured `App::id` while the entity was still alive and to re-offer it
+/// through allows() below - see node_death_detector.cpp's tick() for where that capture
+/// happens and why.
+///
+/// Shared by any detector whose candidate keys are plain strings (node_death's are
+/// `App::effective_fqn()`; a future tf_stale conversion would use its own "parent->child"
+/// pair strings) - the exact-match contract does not care which.
+///
+/// The empty string is never treated specially: a caller that means "match nothing" for
+/// an empty entity_key has to actually insert "" into the allow set for that to happen.
+/// Detector configure() code is expected to have already dropped empty entries when
+/// building the set it hands here.
+class AllowlistSuppressor : public Suppressor {
+ public:
+  explicit AllowlistSuppressor(std::set<std::string> allow) : allow_(std::move(allow)) {
+  }
+
+  /// True iff `candidate` is present, verbatim, in the configured allow set. Public (beyond
+  /// what the Suppressor interface requires) so a caller holding a form of an entity's
+  /// identity that suppresses() itself has no way to reach - `App::id`, captured while the
+  /// entity was still present - can still check it against the same set. See the class doc.
+  bool allows(const std::string & candidate) const {
+    return allow_.count(candidate) > 0;
+  }
+
+  /// Matches `entity_key` verbatim, or the bare leaf of it (the substring after the last
+  /// '/', or the whole key if it carries no '/') - the two forms answerable from the key
+  /// alone. The id form is NOT checked here; see allows() and the class doc.
+  bool suppresses(const std::string & entity_key, const DetectorContext & /*ctx*/) const override {
+    if (allows(entity_key)) {
+      return true;
+    }
+    const auto slash = entity_key.rfind('/');
+    if (slash == std::string::npos) {
+      return false;  // entity_key has no '/': it already IS its own bare leaf, checked above
+    }
+    return allows(entity_key.substr(slash + 1));
+  }
+
+  /// An operator-declared entry is a standing fact about that key for as long as this
+  /// configuration is loaded - it does not start matching and then stop on its own, so a
+  /// key it vetoes may safely have its tracker bookkeeping reclaimed.
+  bool durable() const override {
+    return true;
+  }
+
+ private:
+  std::set<std::string> allow_;
+};
+
+}  // namespace ros2_medkit_graph_watchdog

src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector.hpp

@@ -74,46 +74,62 @@ struct DetectorContext {
       nullptr;                                    ///< entities this tick (id + bound_fqn); null in bare-context tests
   const std::atomic<bool> * cancelled = nullptr;  ///< plugin shutdown flag; a long sweep polls it (null => never)
 
-  void raise_fault(const std::string & code, uint8_t severity, const std::string & description,
+  /// Returns true only once `async_send_request` has actually been called - false for every
+  /// suppression path (Advisory/Off, no client, empty source_id, reliability gate, service
+  /// not ready). This proves the request was handed to rclcpp's client library for sending,
+  /// nothing more: the client is deliberately fire-and-forget (see
+  /// GraphWatchdogPlugin::set_context()'s own note on why - nothing consumes the future), so
+  /// a true return does NOT prove fault_manager received or processed the request, only that
+  /// this call was not one of the silent-decline paths above. A caller that needs to
+  /// distinguish "genuinely attempted" from "merely warranted" (report non-empty) - not
+  /// receipt - must use this return value rather than inferring it from its own inputs:
+  /// node_death_detector.cpp's ever_raised_ guard is exactly that caller, and every one of
+  /// these suppression paths is silent by design (no detector should have to duplicate them
+  /// to know whether it may later trust its own silence as a clear).
+  bool raise_fault(const std::string & code, uint8_t severity, const std::string & description,
                    const std::string & source_id) {
     if (!mode_emits(mode) || !fault_client) {
-      return;  // Advisory/Off suppressed, or client not yet wired.
+      return false;  // Advisory/Off suppressed, or client not yet wired.
     }
     if (source_id.empty()) {
       if (gateway_node) {
         RCLCPP_WARN_ONCE(gateway_node->get_logger(),
                          "graph_watchdog: dropping fault '%s' with empty source_id (detector contract violation)",
                          code.c_str());
       }
-      return;
+      return false;
     }
     if (!reliability_allows(gate, source_id)) {
-      return;  // entity warming up or lifecycle-inactive: suppressed by the reliability core.
+      return false;  // entity warming up or lifecycle-inactive: suppressed by the reliability core.
     }
     if (!fault_client->service_is_ready()) {
-      return;  // fault_manager not reachable yet; avoid unbounded pending_requests_ growth.
+      return false;  // fault_manager not reachable yet; avoid unbounded pending_requests_ growth.
     }
     fault_client->async_send_request(std::make_shared<ros2_medkit_msgs::srv::ReportFault::Request>(
         make_fault_report(source_id, code, severity, description)));
+    return true;
   }
 
-  void clear_fault(const std::string & code, const std::string & source_id) {
+  /// See raise_fault()'s own doc on the return value - identical contract, minus the
+  /// reliability-gate check clear_fault() has never applied.
+  bool clear_fault(const std::string & code, const std::string & source_id) {
     if (!mode_emits(mode) || !fault_client) {
-      return;  // Advisory/Off suppressed, or client not yet wired.
+      return false;  // Advisory/Off suppressed, or client not yet wired.
     }
     if (source_id.empty()) {
       if (gateway_node) {
         RCLCPP_WARN_ONCE(gateway_node->get_logger(),
                          "graph_watchdog: dropping fault-clear '%s' with empty source_id (detector contract violation)",
                          code.c_str());
       }
-      return;
+      return false;
     }
     if (!fault_client->service_is_ready()) {
-      return;  // fault_manager not reachable yet; avoid unbounded pending_requests_ growth.
+      return false;  // fault_manager not reachable yet; avoid unbounded pending_requests_ growth.
     }
     fault_client->async_send_request(
         std::make_shared<ros2_medkit_msgs::srv::ReportFault::Request>(make_fault_clear(source_id, code)));
+    return true;
   }
 };
 

src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector_config_keys.hpp

@@ -13,6 +13,8 @@
 // limitations under the License.
 #pragma once
 
+#include <algorithm>
+#include <cstdint>
 #include <set>
 #include <string>
 #include <vector>
@@ -50,6 +52,50 @@ inline constexpr double min_jump_thresh_sec(int tick_interval_ms) {
   return kJumpThreshTickPeriods * static_cast<double>(tick_interval_ms) / 1000.0;
 }
 
+/// Floor for node_death's own `miss_grace`, in milliseconds of wall clock.
+///
+/// The entity cache a detector reads (ctx.snapshot) is not rebuilt every tick: it is
+/// rebuilt on a graph event debounced to about one refresh per second, so every tick
+/// between two refreshes sees the SAME snapshot. A tick-counted miss_grace therefore does
+/// not measure independent samples of the graph - it can re-count one stale cache
+/// generation as several misses - and that collapses silently the moment the tick period
+/// is shortened: at a 200ms tick, a miss_grace of 2 ticks is only 600ms of nominal grace,
+/// well under a single refresh cycle, so one omitted refresh alone can already cross it.
+/// 3000ms is one debounce cycle plus margin, chosen so the shipped default (miss_grace 2 at
+/// the 1000ms default tick) is unaffected and only a faster-than-default tick ever raises
+/// the effective grace.
+inline constexpr int kMinNodeDeathWindowMs = 3000;
+
+/// Ceiling node_death applies to BOTH `miss_grace` and `prune_grace` - ticks-before-
+/// something-happens knobs that, at the 1s default tick, already mean an hour of silence
+/// before either takes effect at 3600; a value past it is a typo or a unit mix-up, not a
+/// real operator choice. Shared (not detector-local) so graph_watchdog_plugin.cpp's own
+/// compute_departed_retention_ticks() - which has to predict node_death's eventual
+/// prune_ticks_ before that detector's own configure() has run - clamps to the IDENTICAL
+/// bound rather than risking a plugin-side window sized for a value the detector will
+/// itself reject.
+inline constexpr std::int64_t kMaxNodeDeathGraceTicks = 3600;
+
+/// Smallest `miss_grace` (in ticks) that keeps kMinNodeDeathWindowMs of wall clock,
+/// whatever `tick_interval_ms` is configured to. A death is reported once misses EXCEED
+/// miss_grace, i.e. after miss_grace + 1 ticks, so the ceiling divides the window by the
+/// tick period and subtracts the one tick that boundary already buys back.
+///
+/// Computed in int64_t throughout: `tick_interval_ms` is validated only against
+/// `> 0 && <= INT_MAX` (node_death_detector.cpp's own configure()), and at that documented-
+/// valid endpoint `kMinNodeDeathWindowMs + tick_interval_ms - 1` overflows a 32-bit int
+/// before the division ever runs - undefined behaviour at a value the config contract
+/// explicitly accepts. The final result is always small (it shrinks as tick_interval_ms
+/// grows), so narrowing it back to int at the end never loses anything.
+inline int min_node_death_miss_grace(int tick_interval_ms) {
+  if (tick_interval_ms <= 0) {
+    return 0;  // not a real cadence; nothing meaningful to floor against
+  }
+  const std::int64_t wide_tick = tick_interval_ms;
+  const std::int64_t window = static_cast<std::int64_t>(kMinNodeDeathWindowMs) + wide_tick - 1;
+  return static_cast<int>(std::max<std::int64_t>(0, window / wide_tick - 1));
+}
+
 /// Append one warning per key in `detector_cfg` that the detector does not read.
 ///
 /// extract_detector_config() copies EVERY key under detectors.<id> verbatim, and a

src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/graph_watchdog_plugin.hpp

@@ -92,6 +92,15 @@ class GraphWatchdogPlugin : public ros2_medkit_gateway::GatewayPlugin,
   /// clears the pending map as it reads it.
   std::size_t prune_pending_fault_requests_for_test();
 
+  /// Test-only: exposes the otherwise-private compute_departed_retention_ticks() so a unit
+  /// test can drive its config-validation directly (malformed/oversized miss_grace or
+  /// prune_grace) without constructing a full ROS gate/node - it has no ROS dependency of
+  /// its own, only tick_interval_ms_/prune_grace_ (set via configure()+load_parameters())
+  /// and the JSON it is handed.
+  int compute_departed_retention_ticks_for_test(const nlohmann::json & config_snapshot) const {
+    return compute_departed_retention_ticks(config_snapshot);
+  }
+
  private:
   void load_parameters();
   void run_tick_loop();  ///< Body of tick_thread_: tick() + interruptible wait, until shutdown.