feat(graph_watchdog): plugin scaffolding + QoS-mismatch detection - #571
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new ros2_medkit_graph_watchdog gateway plugin package that detects “silent” ROS 2 graph failures (QoS mismatches, orphaned near-miss topic pairs, node disappearance, TF staleness, parameter drift, lifecycle expectations, and timing-budget overruns) and reports them via frozen GRAPH_* fault codes, with a reliability gate to suppress bringup noise and explicit suppression semantics to avoid “silent healing”.
Changes:
- Introduces the graph watchdog plugin scaffolding (detector registry, reliability gate, fault aggregation helpers, and plugin exports) plus the orphan detector implementation.
- Adds a large unit + integration + launch_testing E2E test suite covering detector policies, suppression/pruning behavior, and real gateway+fault_manager end-to-end behavior.
- Re-carves ROS test-domain allocations: gateway shrinks to
30–74, newros2_medkit_graph_watchdogtakes75–89.
Reviewed changes
Copilot reviewed 83 out of 83 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_watchdog_clock.cpp | Unit tests for sim-time vs system-time behavior and monotonicity/validity checks. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_warmup_tracker.cpp | Unit tests for per-entity warmup/forget-grace semantics. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_suppressor.cpp | Unit tests for suppressor-chain behavior and durable default. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_reliability_gate.cpp | Unit tests for warmup + lifecycle gating and status JSON shape. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_orphan_policy.cpp | Unit tests for orphan topic-pair matching rules (namespace/type/system-topic guards). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_node_liveness_tracker.cpp | Unit tests for node disappearance tracking and suppression-gated pruning. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_shutdown_suppressor.cpp | Unit tests for suppressing clean lifecycle shutdowns without hiding error terminations. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_expectation_tracker.cpp | Unit tests for required-active enforcement keyed by node FQN with absence-grace/no-match warnings. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_fault_request.cpp | Unit tests for ReportFault request builder helpers. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_detector.cpp | Unit tests for DetectorContext contracts (null wiring safety) and helpers (tf_static QoS). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_detector_registry.cpp | Tests for static registration and instantiation of detectors. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_detector_config.cpp | Tests for nested detector config parsing, including YAML-boolean off. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_allowlist_suppressor.cpp | Unit tests for exact-match allowlist suppression and durability. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_aggregated_fault.cpp | Unit tests for bounded aggregated fault description generation. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_timing_budget_e2e.test.py | E2E launch test for timing-budget raise/clear via /faults API. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_tf_stale_e2e.test.py | E2E launch test for TF staleness raise/clear with static-pair guard. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_tf_publisher_liveness_e2e.test.py | E2E launch tests for /tf publisher-liveness suppression toggle behavior. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_suppression_allowlist_e2e.test.py | E2E launch test proving allowlist suppression affects aggregated emissions. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_prune_no_false_heal_e2e.test.py | E2E launch test proving pruning cannot heal unsuppressed real deaths. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_orphan_e2e.test.py | E2E launch test for orphan near-miss raise/clear via /faults API. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_node_death_e2e.test.py | E2E launch test for node death raise/clear using real graph node destruction/recreation. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_lifecycle_expectation_e2e.test.py | E2E launch test for required-active lifecycle expectations with real ChangeState transitions. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py | E2E launch test proving nested config plumbing and mode: off behavior end-to-end. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/reliability_gate.cpp | Reliability gate implementation combining warmup tracking and lifecycle state. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/graph_watchdog_plugin_exports.cpp | Plugin C exports for gateway loading and introspection-provider exposure. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/orphan_detector.cpp | Orphan detector implementation with persistence grace and warning-on-unknown-keys. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detector_registry.cpp | Detector factory registry implementation. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/package.xml | New package manifest and dependencies for plugin + tests. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/watchdog_clock.hpp | Sim-time aware clock wrapper with stall detection for time-based detectors. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/warmup_tracker.hpp | Pure warmup/forget-grace tracker used for bringup quiescence. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/suppressor.hpp | Suppressor base class and chain application helper. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/reliability_gate.hpp | Gate API and status JSON surface for the plugin route. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/qos_policy.hpp | Pure QoS RxO compatibility and starvation classification helpers. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/publisher_liveness_suppressor.hpp | Suppressor that vetoes TF-stale when /tf has zero publishers. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/orphan_policy.hpp | Pure orphan matching policy (Levenshtein + guards). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/node_liveness_tracker.hpp | Pure node disappearance tracker with suppression-gated reclamation. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_watcher.hpp | Lifecycle state tracking + departed-state retention for shutdown suppression. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_shutdown_suppressor.hpp | Durable suppressor for clean lifecycle shutdown departures. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_expectation_tracker.hpp | Pure tracker enforcing required-active expectations keyed by node FQN. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/graph_watchdog_plugin.hpp | Plugin public interface and threading/shutdown contract. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/graph_fault_codes.hpp | Frozen GRAPH_* fault-code string constants. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/fault_request.hpp | Header-only ReportFault request builders. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector.hpp | Detector interface + DetectorContext (fault wiring, cancellation, gating). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector_registry.hpp | REGISTER_DETECTOR macro and registry API. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector_config.hpp | Nested detector config parsing helpers (incl. YAML boolean off). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector_config_keys.hpp | Unknown-key warning helper + plugin-injected keys list. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/allowlist_suppressor.hpp | Exact-match allowlist suppressor (durable). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/aggregated_fault.hpp | Aggregated fault emitter + bounded description building. |
| src/ros2_medkit_gateway/CMakeLists.txt | Adjusts gateway test domain allocation range to avoid overlap with new package. |
| src/ros2_medkit_cmake/cmake/ROS2MedkitTestDomain.cmake | Documents new domain range allocation for ros2_medkit_graph_watchdog. |
Comments suppressed due to low confidence (3)
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py:131
- This test is conditionally skipped via unittest.skipUnless. Project test policy here is to avoid skipped tests; please refactor the scenario split so each CTest target executes only non-skipped tests (e.g., separate scenario-specific modules).
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_config_plumbing_e2e.test.py:160 - This test is conditionally skipped via unittest.skipUnless. Project test policy here is to avoid skipped tests; please refactor so the "mode_off_yaml_bool" scenario is exercised by a dedicated test module/target rather than being present-but-skipped in other scenarios.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_tf_publisher_liveness_e2e.test.py:189 - This test is conditionally skipped via unittest.skipUnless. Project test policy here is to avoid skipped tests; prefer a dedicated scenario-specific module/CTest target with no skips for the "live_publisher" case.
Adds ros2_medkit_graph_watchdog, a GatewayPlugin for failures where every node is up, nothing logs an error, and the robot is still broken. This change carries the plugin skeleton and the first detector. The remaining silent-fault classes follow in separate changes, each against its own issue; their codes are already reserved in the frozen GRAPH_* namespace. Scaffolding: - detector interface, self-registering detector registry, nested per-detector config with startup warnings for unknown keys and unknown detector ids - a central reliability gate that holds every raise until an entity has been continuously present for warmup_cycles ticks, so bringup churn does not read as failure - AggregatedFault: one graph-level fault per code, level-triggered, since fault_manager identifies a fault by fault_code alone - one synthetic App published through IntrospectionProvider, so GRAPH_* faults resolve to a real entity and roll up to the host component - a dedicated tick thread, never the gateway executor, since detectors do blocking reads qos_mismatch (GRAPH_QOS_MISMATCH): applies the RxO rule to every publisher x subscriber pair on every topic. Reliability, durability, deadline and liveliness lease are all checked, and duration comparisons saturate instead of wrapping. A subscriber incompatible with every publisher is starved and reported at ERROR; a subscriber incompatible with only some still receives data while one producer is silently discarded, reported at WARN. Affected subscriber FQNs are named in the fault. A persistence threshold absorbs non-atomic endpoint discovery. Test domains 75-89 are carved out of the gateway range, which shrinks to 30-74 and keeps 12 free slots. Each gtest creating live ROS nodes gets its own domain; launch tests share one under a RESOURCE_LOCK. Sharing a domain between gtests let a publisher outlive its test and change the next one's result.
bburda
changed the title
bburda
force-pushed
the
feat/graph-watchdog-plugin
branch
from
July 29, 2026 08:38
d7faf9e to
85d02d1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector_config_keys.hpp:32
- GraphWatchdogPlugin injects
prune_graceinto per-detector configs (graph_watchdog_plugin.cpp:205-207), butplugin_injected_detector_keys()only whiteliststick_interval_ms. Detectors that call collect_unknown_detector_keys() (e.g. qos_mismatch) will therefore warn about the plugin-injectedprune_graceon every startup, even with no operator typo.
/// Keys the PLUGIN injects into every detector's config before calling configure()
/// (GraphWatchdogPlugin::set_context). They are plumbing, not a detector's own keys, so a
/// detector that ignores one must never report it as an operator typo.
inline const std::set<std::string> & plugin_injected_detector_keys() {
static const std::set<std::string> keys{"tick_interval_ms"};
return keys;
}
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/README.md:52
qos_mismatch.allowlistis documented inconsistently: here it says “Subscriber FQNs never reported”, but the detector treats it as topic names (qos_mismatch_detector.cpp checksallowlist_.count(topic)), and the later table in this README also says topic names. Also,graceis implemented as “raise on tick grace+1” (seeif (++streak_[topic] <= grace_) continue;).
| Key | Type | Default | Meaning |
|-----|------|---------|---------|
| `mode` | string | `raise` | As above. |
| `grace` | int | `3` | Consecutive ticks a topic must stay affected before it is reported. Endpoint discovery is not atomic, so a subscriber can be visible before a publisher's QoS is, which reads as starvation for a tick or two. |
| `allowlist` | string[] | `[]` | Subscriber FQNs never reported. Exact match only. |
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/design/graph_watchdog.rst:11
- The design doc claims “the plugin injects no entities”, but the implementation does publish a synthetic App via IntrospectionProvider (graph_watchdog_plugin.cpp::introspect) specifically so GRAPH_* faults are reachable from scoped endpoints. The doc should reflect that, otherwise readers will misunderstand scoping/ownership.
faults via a ``ReportFault`` service client on the gateway node. Faults reach the
SOVD ``/faults`` API via FaultManager - the plugin injects no entities. Its only
HTTP surface is a read-only reliability status route (see `Reliability core`_
below).
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/design/graph_watchdog.rst:19
- This section says the plugin uses a tick wall timer and “No … thread”, but the implementation starts a dedicated tick thread (GraphWatchdogPlugin::set_context creates
tick_thread_running run_tick_loop()). The doc should describe the real threading model since it’s a key operational property (blocking reads).
- **Plugin shell** (``GraphWatchdogPlugin``): loads via the gateway plugin ABI
(v7). In ``set_context`` it casts the context with ``as_ros_plugin_context``,
creates one ``rclcpp::Client<ReportFault>`` on the gateway node, and starts a
tick wall timer on that node. No child node, no extra executor/thread - the
gateway spins its own node.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/design/graph_watchdog.rst:185
- Status section is inconsistent with this PR’s scope: it says “all six silent-fault detector classes are live and raising”, but this change only introduces the scaffolding +
qos_mismatchdetector. This will mislead future maintainers and reviewers about what’s actually implemented.
The plugin loads, ticks the graph, and shuts down cleanly. The reliability core is
real and already ticking, and all six silent-fault detector classes are live and
raising through it. The remaining silent-fault classes land in follow-up changes,
each against its own issue.
The cancel timeout was 5s, hardcoded at the call site. A CancelGoal round-trip is a service call to the action server plus that server's own handling, so its duration is bounded by the server and by machine load, not by us. On a busy CI container the request timed out and the cancel came back as a vendor error even though the goal had been accepted for cancellation. Raise it to 15s behind a named constant, and say at the call site why it is not tighter. Also raise the test step's budget from 15 to 25 minutes: the suite already took 14m13s on lyrical on main, so it was one package away from failing regardless of which package that was. The step is finishing real work, not hanging.
The cancel client budget was below the server's. `delete_request` defaults to a 10s client timeout, while the gateway now allows up to 2s to find the cancel service plus a 15s cancel budget. A slow-but-successful cancel would surface as an opaque ReadTimeout instead of the server's own diagnosable error, so the raised server budget bought less headroom than it looked like. Give the cancel call a client budget that exceeds the server's. The rosbag ring buffer was not given time to refill. A post-fault recording window diverts incoming messages away from the buffer, so a test reporting a fault immediately after a previous test's window closes can find the buffer empty, and flush_to_bag then creates no bag. The file already has a helper for exactly this and two later tests use it; the first two did not, and encoded the assumption in a comment instead. Wait for buffered data before reporting.
mfaferek93
reviewed
Jul 29, 2026
An operation appears under /operations as soon as it is discovered by name, but the gateway cannot send a goal until it has also resolved the underlying ROS interface type. Creating an execution in that gap fails typesupport lookup on the empty type string and returns 500 x-medkit-ros2-action-unavailable. The helper for this already exists and documents the race, but only one test used it. The CI failure pattern matched that split exactly: the subtests using the name-only wait failed, the one using the type-info wait passed, and every subtest after it passed because it had already forced the type to resolve. Point the shared setup helper at the type-info wait, and fix the same latent case in the operations API test.
test_06 reported a fault straight after the previous test's post-fault window closed, with the same empty-ring-buffer outcome the other tests already guard against. Four of the file's buffer-consuming tests now use the helper; none is left relying on poll-loop timing for its margin.
…ument the route The package hand-wrote ROS_DOMAIN_ID on each test instead of using the shared allocator, so the allocation table and the literals could drift apart and exhaustion would have been a silent collision rather than a configure error. Every gtest that creates live ROS nodes now takes its domain through medkit_set_test_domain, from a range this package declares. The launch tests keep an explicit assignment and share one domain under a RESOURCE_LOCK, because the macro writes ENVIRONMENT wholesale and they also carry a port and the plugin path; the comment says so at the call site. Also document GET /api/v1/x-medkit-watchdog in the REST reference. It is operator-facing surface and was the one route the plugin adds without an entry.
…ing wolf Three review findings. The cancel budget ignored service_call_timeout_sec. It was a file-local constant, so cancel became the only call in that file not bounded by the parameter whose own documentation says it applies to every service and action call, and an operator lowering it no longer bounded cancel at all. Derive the budget from the parameter with a floor instead: the configured value wins whenever it is larger, and the floor only protects a cancel from a very small setting. The unknown-key sweep reported the plugin's own injected key. set_context injects prune_grace into every detector config, but the injected-key set listed only tick_interval_ms, so a detector that does not read prune_grace warned about it on every boot with no user config at all. The one mechanism that catches config typos was crying wolf, and an operator acting on it would delete a key they may have set deliberately elsewhere. qos_mismatch could raise a finding nobody could see. The description is capped and affected topics were listed lexicographically, so past roughly five topics a newly starved subscriber on a late-sorting topic changed nothing observable: the code was already raised and the text already full. Name the topics that crossed the persistence gate on this tick first, which is what the ordered emit exists for.
mfaferek93
approved these changes
Jul 29, 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.
Summary
Adds
ros2_medkit_graph_watchdog, aGatewayPluginfor failures where every node is up, nothing logs an error, and the robot is still broken.This is the first of seven changes. It carries the plugin scaffolding and one detector, so the seam is proven end to end before the rest arrive. Each remaining silent-fault class lands separately against its own issue; their codes are already reserved in the frozen
GRAPH_*namespace so the contract does not churn.Scaffolding
warmup_cyclesticks, so bringup churn does not read as failureAggregatedFault: one graph-level fault per code, level-triggered, because fault_manager identifies a fault byfault_codealoneIntrospectionProvider, soGRAPH_*faults resolve to a real entity and roll up to the host componentqos_mismatch(GRAPH_QOS_MISMATCH)Applies the RxO rule to every publisher x subscriber pair on every topic. Reliability, durability, deadline and liveliness lease are checked; duration comparisons saturate instead of wrapping.
Two levels, because they mean different things to an operator:
The fault names the affected subscriber FQNs. A persistence threshold absorbs non-atomic endpoint discovery, where a subscriber can be visible before a publisher QoS is.
Issue
Part of #570.
Type
Testing
18 tests, all green locally on Jazzy: 12 gtest targets (policy units plus integration tests driving real
rclcppnodes and real DDS), 1 launch test running a full e2e against a real gateway process, real fault_manager and a real ROS graph, and 5 linters.Test domains 75-89 are carved out of the gateway range, which shrinks to 30-74 and keeps 12 free slots (it consumes 33 today). Each gtest that creates live ROS nodes gets its own domain; launch tests share one under a
RESOURCE_LOCK. Sharing a domain between gtests let a publisher outlive its test and change the next one.s result.Checklist
Two things worth flagging rather than leaving to be found:
lifecycle_status_helpers.cpp,ros2_lifecycle_state_reader.cpp,ros2_parameter_transport.cpp) becauseros2_medkit_gatewayexports include dirs but installs no library target. Fine in a workspace build, but it would not survive a released source package, so it needs resolving before this is ever bloom-released.flake8andpep257are excluded, matching every other package here: nothing in this repo lints Python today, including the package holding all the other launch tests.copyright,clang_format,cppcheck,lint_cmakeandxmllintare on.