Detect a node that must be active but is stuck in another lifecycle state - #587
Detect a node that must be active but is stuck in another lifecycle state#587bburda wants to merge 5 commits into
Conversation
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. |
d30c31e to
3e798f5
Compare
| // class holds nothing back - and a streak whose absence bookkeeping is gone entirely | ||
| // cannot resurrect as a hold either. | ||
| for (const auto & miss : misses_) { | ||
| if (miss.second <= 0 || miss.second > grace_) { |
There was a problem hiding this comment.
The comment is doing the damage here: past-grace implies "already in affected" only when the node was seen this tick. report.affected is written at :184 inside the loop over nodes, which is built from matches.
On an absent tick the node is in neither set: not in seen_fqns so not in affected, misses_ preserved by the absence loop above, skipped here so not in pending. In the detector guard.read is sticky (lifecycle_expectation_detector.cpp:351 is the only true-assignment) so the unread leg at :371 excludes it, and ever_matched_ keeps unmatched_blocking false. All three legs of the guard at :379 stay quiet and emit(ctx, {}) sends EVENT_PASSED on the very first blink, while the absence loop right above is deliberately preserving the streak for 3 ticks.
I compiled the header standalone (grace=1, absence_grace=3) to be sure:
present tick 1 affected=1 pending=0 <- reported
ABSENT tick 1 affected=0 pending=0 tracked=1
ABSENT tick 2 affected=0 pending=0 tracked=1
ABSENT tick 3 affected=0 pending=0 tracked=1
ABSENT tick 4 affected=0 pending=0 (streak erased - by design)
So a node measured stuck and already reported gets strictly less protection than one below grace, which PendingStopsWhenAbsenceOutlivesTheAbsenceGrace pins as holding through absent ticks 1-2. Under bringup_params.yaml the hysteresis latch absorbs a single blink, but it still stamps last_passed on a still-violating fault and burns the healing debounce; at healing_threshold: 0 or 1 (the value the package README's own config block recommends) it flips to HEALED.
Gating on presence keeps both behaviours:
if (miss.second <= 0 || (miss.second > grace_ && seen_fqns.count(miss.first) != 0)) {Patched a copy and re-ran: absent ticks 1-3 hold the clear, tick 4 hands over as designed. PendingStopsWhenAbsenceOutlivesTheAbsenceGrace and AbsenceAfterRaiseClearsNotNodeDeathsDomain both still pass. Dropping the > grace_ term outright would break the first.
|
|
||
| // A matched node that left the snapshot (or came back unread) keeps its streak for | ||
| // absence_grace_ ticks; past that the streak is discarded, since absence itself | ||
| // belongs to the presence class (GRAPH_NODE_DISAPPEARED). |
There was a problem hiding this comment.
This handoff is load-bearing in several places (also lifecycle_expectation_detector.cpp:243-251 and :296-300, plus AbsenceAfterRaiseClearsNotNodeDeathsDomain), but GRAPH_NODE_DISAPPEARED exists only in this PR's test comments. No code definition, no registered detector (REGISTER_DETECTOR yields exactly qos_mismatch, orphan, param_drift, lifecycle_expectation), nothing under any branch. graph_watchdog_plugin.cpp:44 already reads detectors.node_death.miss_grace and cites a node_death_detector.cpp that is not in the tree.
So the terminal case today is: required node stuck inactive raises correctly, its process then dies outright, everything clears, nobody reports it. Fine if node_death is landing right behind this - just worth saying so on the PR, or landing them together.
| // namesakes are both reported instead of one silently replacing the other. | ||
| matches.push_back(LifecycleMatch{id, fqn, state}); | ||
| bool & read = matched_read[fqn]; | ||
| read = read || (state.has_value() && !state->empty()); |
There was a problem hiding this comment.
read collapses two cases the tracker deliberately keeps apart: nullopt (no lifecycle to be wrong about, lifecycle_expectation_tracker.hpp:251) and "" (a seed that has not answered, aged like absence at :173). Both land as !read and both get the same kUnmeasuredHoldTicks bound.
That bound is right for nullopt - a typo must not block healing forever, which is what the comment at :337-341 argues. It is wrong for "". A managed node hung inside on_configure on a single-threaded executor cannot serve get_state, and its configuring transition event went out before the volatile subscription matched. LifecycleWatcher seeds once plus kReseedAttempts = 2 (lifecycle_watcher.cpp:48, budget set at first sighting and never replenished), all timing out, so the label stays "" for the life of the process. Then: the tracker continues at :173 so no streak ever starts and pending is empty, guard.unmeasured freezes past 60 and drops out of the scan at :371, unmatched_blocking is false because the entry did match - and from tick 61 on the detector emits a clear every tick.
That is a positive "this node is fine" about a node it has never measured, which is the failure mode the detector exists to catch. Suggest tracking the two apart: keep the bounded release for nullopt, and for "" either keep withholding or re-warn instead of going quiet.
Worth noting the integration fixture cannot catch this - its apps carry no services (test:30-34), so a node without an injected label is simply absent from LifecycleWatcher::tracked and Some("") never reaches the guard.
| } | ||
| withheld_ticks_ = 0; | ||
| withheld_reported_ = false; | ||
| aggregated_.emit(ctx, report.affected); |
There was a problem hiding this comment.
This is the only aggregating detector calling plain emit(). orphan_detector.cpp:187, qos_mismatch_detector.cpp:203 and param_drift_detector.cpp:1068 all pass a new-first order to emit_ordered so the 480-char cap cannot hide a fresh entity.
The cap applies here too, source_id is the constant graph_watchdog, and the record is keyed by code alone - so the description is the only thing carrying which node broke. A detail runs ~97 chars, so 4-5 fit. With require_active: ["controller_server"] over /robot01../robot12: once robot01-05 are stuck and CONFIRMED, robot12 going inactive produces a byte-identical description and is never named. TwentyFiveStuckNodesRaiseOneCappedFault pins the lexicographic order, so I assume this was deliberate - but the consequence looks unintended.
Same line, secondary: *node.state is msg->goal_state.label taken verbatim off a remote ~/transition_event (lifecycle_watcher.cpp:272). The cap keeps it out of the store, but with no per-node trim (param_drift has one at :1064) one long label eats the whole budget and masks every other affected node.
… sources No behaviour change. These were not reported by any gate: the package registers no clang-tidy ctest target, and the CI job that does run over changed files passes no -warnings-as-errors.
…, not its app id The shared lifecycle cache keyed its tracked map by App::id. An id can stay the same across a graph sweep and point at a different node, and the cache then kept the old node's state and its old transition subscription, so a consumer could be handed one node's lifecycle label for another node. An entry is now identified by the pair (fqn, GetState path), both recorded when the node is first seen and compared on every update. When the pair changes the old binding is recorded as a departure under its own fqn, the entry is dropped, and the id is seeded again like any newly discovered node.
The operator lists the nodes that must be active; the detector reports the ones that are not. With no require_active entries it does nothing at all, so it cannot false-positive on a graph nobody configured. Three fault codes, because "this node is not active" and "I cannot tell whether this node is active" are different operator problems and the fault store keys a record by fault code alone: GRAPH_NODE_INACTIVE (ERROR) when the state is measured and is not active, GRAPH_NODE_UNREADABLE (WARN) when a managed node's state cannot be read at all, GRAPH_NODE_NOT_MANAGED (WARN) when the node named has no lifecycle to read. A node is content of at most one at a time. Per node per tick the tracker classifies one observed state and keeps two clocks. The violation streak advances on inactive and resets only on active. The unmeasured clock advances on either unreadable or not-managed and resets only on a real measurement; it is deliberately blind to which of the two it saw, because a counter per cause lets a node flapping between them reset each counter with the other and stay invisible to both. Sustained absence continues whichever clock the node's last settled observation put it on rather than erasing it, so a node in a restart loop is reported instead of evading every code forever. Absence after an active observation continues nothing: reporting a healthy departure belongs to GRAPH_NODE_DISAPPEARED, which has no detector yet. The classification is settled over several ticks, since a healthy node reads as unmanaged for one sweep whenever its get_state path is missing from that sweep. Bookkeeping is bounded by evidence rather than age: prune_grace reclaims only idle entries, and tracked_node_cap bounds the map. At the cap idle entries are reclaimed first, then entries for departed nodes are collapsed into a count that still keeps the code raised, and only then is a newcomer refused - so a present node always wins a slot. A refusal also withholds the GRAPH_NODE_INACTIVE clear, and saturation is reported on GET /x-medkit-watchdog rather than only in the log. A clear has to prove as much as a raise: the clear is withheld while an entry has not matched a node, or a matched node has never been read, or a matched node reads not-active but has not passed grace. Every hold is bounded, so a typo in require_active cannot block healing forever. Closes #585
f5b99aa to
ed5b7c8
Compare
The step was being killed mid-package, which surfaces as a test failure and hides every test the kill cut off. Raise the job cap alongside it, or the job cap kills the run before the step's own cap can.
…the fault surface is up The blink scenario confirmed a node's return from the watchdog's entity list, which ReliabilityGate builds from WarmupTracker entries - and those are retained for a grace after the node stops being present. The poll therefore matched the entry left from before the SIGTERM and returned while the replacement process was milliseconds old, so the absence it measured described a node the tracker never saw leave. Confirm the return from GET /apps, which carries no such retention, and poll for the process id rather than reading it once, since launch updates that from its own event loop. The absence also has a lower edge that was never asserted: shorter than one tick interval it can fall between two samples, so the node is never observed away and the blink exercises nothing. Assert both edges and raise the respawn delay past one interval so the window is reachable by construction. Silence windows fail on any transport error, and the first GET /faults of a test also pays service discovery to the fault manager - which on the distro running default FastDDS can outlast the window's own per-poll timeout. Prove the fault surface answers before entering the window, in each class that has one, since every class runs as its own test target in its own process.
Summary
Adds the
lifecycle_expectationdetector to the graph watchdog plugin. The operator lists thenodes that must be active, and the detector reports the ones that are not. With no
require_activeentries it does nothing at all, so it cannot false-positive on a graph nobody configured.
Config keys under
detectors.lifecycle_expectation:require_active(list, empty by default),grace(default 5, accepted 0..300),tracked_node_cap(default 512, accepted 1..16384), plus theplugin-wide
prune_graceandmode. An entry matches 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.
graceis capped at 300 rather than at the int range because it also decides how long a node thatleft the graph while not-active sits unsettled, and
GRAPH_NODE_INACTIVE's clear is withheld forevery node while it does. At an unbounded maximum the code could neither raise nor heal for weeks.
Three fault codes, because "this node is not active" and "I cannot tell whether this node is
active" are different operator problems and the fault store keys a record by fault code alone:
GRAPH_NODE_INACTIVEactiveGRAPH_NODE_UNREADABLEGRAPH_NODE_NOT_MANAGEDA node is content of at most one of the three at a time. Sharing one record would mean a single
unreadable node keeps it raised forever while a healed node never receives a debounce step toward
healing.
One state machine, because counters cancel each other
Per node, per tick, the tracker classifies exactly one observed state -
ACTIVE,INACTIVE,UNREADABLE,NOT_MANAGED, orABSENT- and keeps exactly two clocks:INACTIVE, resets ONLY onACTIVE, reported pastgraceUNREADABLEorNOT_MANAGED, resets ONLY on a realmeasurement. It is deliberately blind to which of the two causes it saw
That blindness is the point. With one counter per cause, a node flapping between two causes resets
each counter with the other cause and stays invisible to both. A cause-blind clock keeps advancing
instead, which closes every such pair at once, including pairs nobody has enumerated.
When the unmeasured clock matures, its code takes the node and the violation streak is released, so
GRAPH_NODE_INACTIVEcan heal for it. The node returning toINACTIVEre-earns grace.Absence continues a violation; it does not erase one
Any rule that erases accumulated evidence is a hole for a node that periodically triggers that rule.
A node in a restart loop - start, crash, respawn delay, start - triggers absence by construction,
and it is the case this detector most exists to catch. So:
ACTIVEINACTIVEUNREADABLE/NOT_MANAGEDUp to
absence_grace(a fixed 3 ticks) everything is held unchanged, so an ordinary blink costsnothing. Past it the clocks resume rather than restart.
Two consequences worth stating plainly for anyone consuming these faults. A departure never heals
a fault, within a gateway lifetime: a node that leaves while violating stays reported until it
returns and reads
active, or until the operator changes the configuration. A gateway restartre-baselines, because the restarted detector has no measurements and an entry that matches nothing
cannot be told apart from a misspelt one; that boundary is pinned by a scenario rather than left to
be discovered. A departure never starts one: a node measured
activethat shuts down raisesnothing. Reporting a healthy departure is what
GRAPH_NODE_DISAPPEAREDowns; it has a fault codebut no detector yet, and #570 sequences that work behind the suppression framework it needs.
Because "healthy departure" decides whether a fault is raised at all, the classification does not
turn on a single tick. A present, healthy, managed node reads as unmanaged for one sweep whenever
its
get_statepath is missing from that sweep, and without settling, one such tick before a cleanshutdown would leave a permanent fault about a node that was fine.
Bookkeeping is bounded by evidence rather than by age.
prune_gracereclaims only IDLE entries -both clocks at zero and no matured ownership, so nothing to lose - and an entry carrying evidence is
never reclaimed by age however long the node stays gone.
tracked_node_capbounds the map instead,and the order at the cap matters: idle entries are reclaimed first, then entries for departed nodes
are collapsed into a count that still keeps the code raised, and only if every tracked node is
present and carrying evidence is a newcomer refused. A present node therefore always wins a slot.
Without that order, a fleet whose robots come and go under their own namespaces fills the map with
the dead, and a genuinely broken present node is refused, never checked, and silently reported
healthy - the exact failure this detector exists to prevent.
Refusing to track a required node also withholds the
GRAPH_NODE_INACTIVEclear: a detector thatdeclined to check a node cannot assert that every node is healthy. Saturation is reported on
GET /x-medkit-watchdogunderdetectors.lifecycle_expectation, not only in the log, and it isre-armed when it ends so a later real saturation is not silenced by an earlier one.
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.
GRAPH_NODE_INACTIVEnow sends nothingwhile an entry has not matched a node yet, or a matched node's state has never been read, or a
matched node reads not-active but has not passed
grace. A raise is never withheld, and each holdis bounded, so a typo in
require_activecannot block healing forever.Without this a gateway restart heals a fault that is still real: the stored fault is
CONFIRMED,the restarted detector has empty state, and the clears it sends before the first read are enough to
reach
HEALED. There is an e2e for exactly this - it kills the gateway by PID, waits for the portto go down, waits for the plugin to arm again, and fails if a
PASSEDarrives while the node isstill inactive.
A node can be replaced under the same app id
The shared lifecycle cache keys its tracked map by
App::id. An id can stay the same across a graphsweep and point at a different node, and the cache then kept the old node's state and its old
transition subscription. An entry is now identified by its binding instead: the pair (fqn,
GetStatepath), both recorded when the node is first seen.update()compares that pair everytick. When it changes, the old binding is written down as a departure under its own fqn, the entry
is dropped, and the id is seeded again like any new node. For this detector that means the old
node's label is never applied to the node the id points at now.
The
orphanandqos_mismatchdetectors are untouched apart from one comment fix each.A separate commit clears 20 clang-tidy findings in this package that no gate was reporting. They
have nothing to do with the detector. Why the package has no clang-tidy target, and what it would
take to add one, is written up in #605.
Issue
Type
Testing
Full package suite green on Jazzy locally: 611 tests, 0 failures, 44 skipped (the skips are
cppcheck). 66 tracker unit cases, 91 integration cases, 15 in the lifecycle cache suite, and 12 e2e
scenarios for this detector across 84 cases.
Everything above is driven end to end rather than argued. Two fixture nodes make the awkward states
reachable on a real stack: one advertises the lifecycle services and holds its
GetStateresponsesopen without ever answering, the other drops its own lifecycle services on command and then exits.
Between them the e2e covers a sustained read failure and its clear by read; a node that leaves and
whose fault survives the departure; a crash loop of twelve real SIGTERM/respawn cycles, each proven
absent past the grace via
GET /apps; a cap saturated by departed nodes, where a present brokennode must still be reported and no clear may be emitted while one is refused; a healthy node whose
services blink for one sweep before a clean shutdown, which must raise nothing while a genuinely
unmanaged departure still does; a configuration at the old unbounded
grace, which must be refused;and a gateway restart with a departed node's fault outstanding, which pins the re-baseline boundary.
The tests sweep the config space at both ends of every documented range (
graceat 0 and 300 plusone past each,
tracked_node_capat 1 and 16384 plus one past each,prune_graceat 0, 1, 3600 and3601, an empty and a non-array
require_active, unknown and misspelt keys), scale past thedescription cap and past the configured tracked-node cap, and change during a run: nodes appearing
late, vanishing after being reported, returning under a new binding, returning after their entry was
collapsed, and alternating between every pair of unmeasured causes.
Every scenario that asserts a fault is ABSENT proves the channel it watched was alive for the whole
window, not just at the start, and distinguishes "asked, and there is no such fault" from "could not
ask". The harness carries a test of that test: it points a scenario at a fault surface that dies
mid-window and fails if the silence assertion passes.
Reviewers can check the restart behaviour with
ctest -R test_lifecycle_expectation_e2e_main, thecrash loop with
ctest -R test_lifecycle_expectation_e2e_restart_loop, and the config validationwith
ctest -R test_lifecycle_expectation_integration.Checklist