Detect a node that must be active but is stuck in another lifecycle state - #587
Open
bburda wants to merge 1 commit into
Open
Detect a node that must be active but is stuck in another lifecycle state#587bburda wants to merge 1 commit into
bburda wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new graph_watchdog detector, lifecycle_expectation, to detect managed lifecycle nodes that are present in the ROS graph but fail to reach active within a configurable grace period, raising GRAPH_NODE_INACTIVE. It also strengthens the shared lifecycle state cache to correctly handle binding reassignments (same app id pointing to a different node/service path) and to avoid stale GetState reads overwriting fresher transition events.
Changes:
- Add
lifecycle_expectationdetector + pure tracker (LifecycleExpectationTracker) with bounded “withheld clear” semantics to prevent restart-induced false heals. - Harden
LifecycleWatcheridentity handling (re-bind detection) and prevent stale re-seed results from overwriting newer~/transition_eventlabels. - Add extensive unit/integration/e2e test coverage, plus documentation/design/changelog updates and test-domain/CMake wiring.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/lifecycle_expectation_detector.cpp | New detector implementation and config validation for lifecycle expectation enforcement and withheld-clear behavior. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_expectation_tracker.hpp | New pure tracker implementing per-node grace/absence/no-match tracking and bounded bookkeeping. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/lifecycle_watcher.cpp | Fix lifecycle cache re-bind handling and prevent stale GetState results from overwriting fresher transition events. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_watcher.hpp | Extend tracked identity to include GetState path + label epoch to support safe rebinds and stale-read suppression. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_watcher.cpp | Add regression tests for rebind identity semantics and stale-read vs transition-event ordering. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_expectation_tracker.cpp | Unit tests for the pure lifecycle expectation tracking logic and edge cases. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_expectation_integration.cpp | Integration tests driving the detector against a real ReliabilityGate and fake ReportFault service. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/test_lifecycle_expectation_e2e.test.py | Full-stack e2e scenarios (raise/heal, default-config silence, negative control), including restart behavior. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/harness.py | Add faults-surface liveness gate and support optional gateway respawn for restart-focused scenarios. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/qos_mismatch_detector.cpp | Minor refactor/comment adjustment (no functional detector changes). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/orphan_detector.cpp | Minor comment adjustment (no functional detector changes). |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/README.md | Document the new detector keys, semantics, and test tiers; update package capabilities list. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/design/graph_watchdog.rst | Update design documentation for lifecycle_expectation and refined lifecycle watcher behavior. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/graph_watchdog_plugin.hpp | Clarify set_context() precondition and threading publication expectations. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/CMakeLists.txt | Add new tests + e2e targets, adjust test domain allocation to avoid ROS_DOMAIN_ID collision, and wire new sources. |
| src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/CHANGELOG.rst | Add package changelog entry referencing the new detector and overall plugin features. |
| src/ros2_medkit_integration_tests/ros2_medkit_test_utils/launch_helpers.py | Add gateway respawn support for restart scenarios in launch tests. |
| docs/design/index.rst | Link graph_watchdog design documentation into global design index. |
| docs/changelog.rst | Include graph_watchdog package changelog in aggregated docs changelog. |
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 the
lifecycle_expectationdetector to the graph watchdog plugin. The operator liststhe nodes that must be active, and the detector raises
GRAPH_NODE_INACTIVEwhen one ofthem is present but stays in another lifecycle state for longer than
graceticks. With norequire_activeentries the detector does nothing at all, so it cannot false-positive on agraph nobody configured.
Config keys under
detectors.lifecycle_expectation:require_active(list, empty bydefault),
grace(default 5), plus the plugin-wideprune_graceandmode. An entrymatches by app id, by full FQN, or by bare node name, so one entry can cover several
namespaces. Unknown keys and out-of-range values produce a startup warning instead of being
dropped in silence.
Two things are not obvious from the diff.
A clear has to prove as much as a raise. The aggregated fault is level triggered, so an
empty result would normally send a clear every tick. That is wrong when the detector has not
measured anything yet. The detector now sends nothing at all while any of these is true: an
entry has not matched a node yet, a matched node's state has never been read, or a matched
node reads not-active but has not been in that state long enough to pass
grace. A raise isnever withheld. Each hold has a limit, so a typo in
require_activecannot block healingforever, and a node that stays absent past the absence grace stops blocking, because long
absence belongs to the presence class.
Without this a gateway restart heals a fault that is still real: the stored fault is
CONFIRMED, the restarted detector has an empty state, and the clears it sends before thefirst read are enough to reach
HEALED. There is an e2e for exactly this: it kills thegateway by PID, waits for the port to go down, waits for the plugin to arm again, and fails
if a
PASSEDarrives while the node is still inactive.A node can be replaced under the same app id. The shared lifecycle cache kept the old
node's state and its old transition subscription in that case. It now treats a changed
binding (fqn or get_state path) as a new node: the old binding is recorded as departed, the
entry is dropped, and the state is read again from scratch. The same file also had a bug
with stale reads: a blocking
GetStateanswer could overwrite a newer~/transition_eventthat arrived while the read was in flight, which at bringup couldleave a label that stays wrong for the rest of the process. A re-read result is now
dropped if an event landed for that entry in the meantime.
The
orphanandqos_mismatchdetectors are untouched apart from one comment fix each.Issue
Type
Testing
23 tracker unit cases, 45 integration cases, 14 lifecycle cache cases, and one e2e file with
three CTest targets (raise and heal with real lifecycle transitions, the default config
staying silent, and an already-active node never raising). Full package suite is green on
Jazzy locally: linters 5/5, non-e2e 23/23, e2e 10/10.
The tests cover the config space at its ends (grace at 0 and past the int range,
prune_graceat 0, 1, 3600 and 3601, an empty and a non-arrayrequire_active, unknown andmisspelt keys), the scale case (25 stuck nodes produce one fault whose description stays
under the cap), and change during a run (a node appearing late, a node vanishing after it
was reported, reconfiguration while counters are live).
Every e2e scenario that asserts a fault is absent checks three things first: the plugin is
armed, the faults endpoint answers, and the node's lifecycle state was really read. Without
those checks the same assertions also pass when the stack never started, so the test would
prove nothing.
Reviewers can check the restart behaviour with
ctest -R test_lifecycle_expectation_e2e_main, and the config validation withctest -R test_lifecycle_expectation_integration.Checklist