Detect a node that dies when nothing else is left to report it by bburda · Pull Request #625 · selfpatch/ros2_medkit

bburda · 2026-08-19T15:49:32Z

Pull Request

Summary

A node dies and nothing reports it. The node cannot report it itself, and the other detectors
cannot see it: qos_mismatch needs both sides present to compare profiles, orphan looks at a
topic whose publisher took the topic with it, and lifecycle_expectation only watches nodes the
operator listed as required-active.

This adds node_death, which raises GRAPH_NODE_DISAPPEARED for a node that was alive and armed
and stopped being alive, together with the suppression framework the umbrella issue asks to be
opt-in and explicit.

Alive is not membership in the entity snapshot. In runtime discovery a dead node leaves the
snapshot, so the two look the same; in manifest and hybrid discovery the manifest keeps the App
and only clears its online flag, so a detector counting membership would make a manifest node
immortal. Tracking is keyed on the stable fully qualified name rather than the app id, because an
id is recomputed each sweep and gains a namespace prefix once a bare-name collision exists, so a
live node held under its old id would be reported as gone.

The grace period is wall clock, not ticks. The entity cache is rebuilt on a debounced graph
event, so every tick between two refreshes sees the same snapshot and one absent cache generation
is counted again on each of them. At a fast tick a two-tick tolerance is shorter than one refresh
cycle, so the window has a floor in milliseconds and says so when it raises the configured value.

Suppression is opt-in. Nothing is suppressed unless suppress names it. A configured
allowlist that suppress does not name has no effect and warns that it has none. Two
suppressors ship: an operator allowlist, and clean lifecycle shutdown. A suppressor also declares
whether its veto is durable, because only a durable veto may reclaim tracker bookkeeping - a veto
that can lift later would otherwise lose a real fault for good once the condition ends.

Also in this change: the absence boundary

GRAPH_NODE_INACTIVE says a required node is not active. GRAPH_NODE_DISAPPEARED says a node is
gone. Both can be true of one node, they have different repairs, and both standing at once is
correct - this change does not remove that.

What changed is narrower. Sustained absence used to mature a violation that had not yet been
reported, so a node observed non-active for a few ticks and then killed acquired an inactive fault
built entirely from evidence gathered after it could no longer be observed. That rule existed
because no presence detector existed to own departures. Now one does, so absence continues a
violation that has already matured and no longer creates one that has not.

With one exception, which is the part worth reviewing closely: the reliability gate refuses to arm
a managed node that never reads active, and node_death only tracks armed nodes. For such a
node the handover has no receiver, so absence may still mature its violation - otherwise a
required node that comes up unconfigured and dies is reported by nothing at all. The split is
keyed on whether the node was ever armed, latched rather than recomputed, because a managed node
legitimately reads not-armed again after an ordinary deactivate while node_death is still
tracking it.

Known limitation

A second node's death, while the fault from the first is still outstanding, is added to the
fault's description but arrives as an update to an already-confirmed record: no state transition,
no freeze frame and no recording of its own. Acknowledging the fault between the two deaths avoids
this, because a report arriving after an acknowledgement is counted as a new occurrence and gets
its own evidence.

Two ways of forcing it from the detector were measured and rejected rather than skipped. Sending a
clear and a raise back to back does nothing: a single opposite-direction report cannot move the
fault manager's hysteresis latch, so the raise lands on the already-confirmed branch. Calling the
clear service instead is worse than not doing it, because it deletes the per-topic readings
captured for that fault and by default clears every symptom the correlation engine attributes to
it. A correct fix belongs in the fault manager, as an operation that re-confirms a record without
clearing it. This is documented in the package README and design doc.

Also in this change: presence ownership rests on knowledge

Two detectors divide one job, and until now the division was decided by a fact the plugin did not
have. GRAPH_NODE_DISAPPEARED is raised for a node the reliability gate has armed; the gate arms a
managed node whose lifecycle state reads active, and it also arms one whose state has never been
measured, because it deliberately treats an unknown state as "do not gate" so that a node nobody
can read is not silenced across every other detector. Those two answers are not the same fact, and
the second was being latched permanently into the flag lifecycle_expectation consults before it
lets absence mature a violation. A required node that never configured could therefore be reported
by the presence detector and, by the same latch, not reported by the only detector that could still
have described what was wrong with it.

The division now asks a second, stricter question, and the gate itself is unchanged. Ownership is
earned when the node is not managed at all or its label reads active; it is withheld while the
state is merely unread and the watcher still has re-seed attempts left; and once those attempts are
spent it is provisional rather than earned - the node is owned, because otherwise nothing would
ever report its departure, but that grant yields the moment a real label finally arrives. A grant
made on knowledge latches, because a later deactivate does not hand the node back. A grant made on
ignorance does not, because the reason for it stops being true the instant somebody else can report
the node.

One consequence is worth stating rather than leaving to be found: a node that departs during its own
warmup is reported by nobody. That is the bringup-quiesce trade-off behaving as designed, it
predates this change, and it is written down where the boundary is described.

Also in this change: the sanitizer test budget

The sanitizer jobs walk the workspace one package at a time under a single 45-minute budget for
the whole step. This package's end-to-end suite runs 24 minutes of that under instrumentation, up
from 10:38 on main, so the step stopped fitting: the TSan run was cut inside the package that
follows this one, and ASan finished the same run with 20 seconds to spare. No single scenario is
responsible - the slowest is 106 s and the rest is a 30-50 s tail across 73 targets, because these
tests wait on grace windows, respawn delays and restart loops rather than compute.

quality.yml now tests this package in a job of its own, once per sanitizer, and ci.yml does the
same over humble and lyrical, where the same cap applies to the step rather than to the job: lyrical
was cut inside this package and humble finished with three minutes to spare. The sweeps skip it. The job builds only the chain up to the package, which still includes the
integration-test package the scenarios launch from, and restores the matching sweep's ccache
instead of saving a second copy of the same objects. Instrumentation, the timeout multiplier and
the sanitizer options are unchanged from the sweep it left, and the job is not path-filtered,
because the plugin drives the gateway, the fault manager and discovery, so the changes most likely
to break it are outside its own tree.

The scenarios' own wall-clock budgets now scale with MEDKIT_TEST_TIME_SCALE, which the sanitizer
jobs already set to the factor they apply to every declared CTest timeout. A budget asserted inside
a test is invisible to that rewrite, so an instrumented graph that is slow to forget a departed node
blows one and the failure reads as a detector that never reported. Only give-up bounds scale: poll
intervals, enforced respawn delays and the sustained-observation windows keep their values, and the
scale is 1.0 whenever the variable is unset, so the normal jobs are unchanged.

Issue

closes A node dies and nothing is left to report it #624

Type

Bug fix
New feature or tests
Breaking change
Documentation only

Testing

The tests were written and merged before the detector, so every scenario was watched failing for
the right reason first, and the suite is what the implementation had to satisfy rather than a
description of it.

73 ctest targets, all green, of which 42 are end-to-end against a real gateway, a real fault
manager and a real ROS graph. 23 of those scenarios are new here.

The end-to-end set covers: a process that exits, its return and the clearing that follows, a
managed node that deactivates but keeps running, a manifest node that never came online, CLI-
convention node names, two nodes sharing a bare name in different namespaces, a fast tick against
a stale cache generation, a node restarting repeatedly across a window, a gateway restart with a
death outstanding, each suppressor including the inert-allowlist warning, pruning that must not
heal a fault that is still true, and each side of the absence boundary.

Every scenario that asserts an ABSENCE gates on the plugin being armed first and uses a helper
that fails when the fault surface is unreachable, so it cannot pass against a stack where the
plugin never loaded. Two such helpers are added here, each with a test that points it at a surface
which dies mid-window and fails if the helper still passes.

The whole suite was also run with the plugin path pointed at a file that does not exist: every
scenario fails, naming the load failure.

Reviewers can check the boundary with ctest -R "node_death_boundary_e2e", the suppressors with
ctest -R "node_death_suppression_e2e", and the tracker's bound with
ctest -R "test_node_liveness_tracker".

No route is added or changed. The only public surface difference is additive content inside the
existing x-medkit-watchdog payload, which gains a node_death block.

Checklist

Breaking changes are clearly described (and announced in docs / changelog if needed)
Tests were added or updated if needed
Docs were updated if behavior or public API changed

…ector Gate the six perturbation scenarios on the per-node app_id-scoped arming signal instead of the global one, so a kill cannot land before the detector has actually observed the specific node being perturbed. Rebuild ros2cli_ignored around what the future detector's own exclusion actually checks: a leaf node name starting with '_ros2cli_', regardless of what process produced it. test_01 (load-bearing) arms and kills an ordinary, long-lived demo node renamed to carry that prefix - a normal, deterministically discoverable target, unlike an ephemeral `ros2 topic echo` process racing DDS discovery on its own short lifetime, which failed in different, unrelated-looking ways across repeated runs. That node stays invisible to the gateway's generic internal-node filter only if `discovery.runtime.filter_internal_nodes` is left on, so this scenario turns it off - otherwise the scenario would pass for the same wrong reason as an untracked node: never tracked at all, rather than tracked and then correctly excluded by name. The renamed node is spawned via its installed binary path rather than `ros2 run`: ros2run's own run_executable() forks the target as a second subprocess and only ever forwards SIGINT to it, so a SIGTERM aimed at the wrapper's pid kills the wrapper and orphans the real node underneath it instead of reaching it. test_02 replaces the live-process realism check (tried twice, unreliable both times on this stack) with a deterministic comparison against the installed ros2cli package's own naming constants, so the fixture's prefix cannot silently drift from what ros2cli itself actually uses. Document why fast_tick_floor does not force the stale graph-cache- generation condition it names: the race is between the 100ms poll timer and the debounced refresh, not something a test can pin without new subprocess-lifecycle machinery, so the docstring states the weaker, actually-proven claim instead. Correct the timeout-budget comments for manifest_never_online (two 30s polls, not one) and deactivated_not_dead (three separate 30s budgets), and recompute every other comment whose internal arithmetic moved as a consequence of the app_id-gating and ros2cli_ignored changes. No TIMEOUT value changes except ros2cli_ignored, which grows with the real cost of proving CLI-node presence deterministically.

…head of its detector Fix ten defects in the suite: two rows a correct detector could not have satisfied (lifecycle_clean_shutdown never enabled the suppressor it exercises; the restart-loop row demanded an outage shorter than the detector's own wall-clock floor), six rows that checked only the fault code and would pass a detector that misattributed the fault to the wrong entity, one row whose precondition was inferred from elapsed time rather than read from an observable, one whose sampled window was never proven to sit before arming, and one whose two comparison windows ran sequentially instead of overlapping. Also corrects a stale timeout-budget comment and an inaccurate cross-reference in the module docstring. assert_fault_never_names is a new harness helper, with its own test-of-the-test, for the one claim ("no disappearance names this node") that none of the existing three window assertions expresses without also being satisfiable by a detector that never runs at all. The config-endpoint row is single-gateway, single-domain: the claim is only that miss_grace changes an observable, which needs one gateway checked at two points on one timeline, not two gateways compared side by side. The two-gateway version surfaced a real, reproducible gap in the fault-service transport that is unrelated to this suite and is not investigated here.

Adds the node_death detector (GRAPH_NODE_DISAPPEARED for an armed App whose bound node vanishes) and the shared Suppressor framework two already-merged e2e suites are waiting for: an interface with a durable() flag, an allowlist implementation, and a lifecycle-clean-shutdown implementation that reads the reliability gate's departed-lifecycle state. The aggregate is level triggered with no forced clear-then-raise; a suppressor may only feed the liveness tracker's prune() when it is durable, so a condition that can lift never silently reclaims a still-real death. status_json() exposes tracked_count on GET /x-medkit-watchdog through an atomic snapshot published once per tick, since the tracker itself carries no synchronization against the HTTP handler thread that reads it. AllowlistSuppressor matches an entity key three ways, mirroring lifecycle_expectation_detector.cpp's own require_active matching: the key verbatim, its bare leaf (an operator naturally writes the bare node name, not its full namespaced fqn), and - through a new allows() method, called from node_death_detector.cpp's tick() while an entity is still present - its App::id, which a same-bare-name collision elsewhere in the graph can prefix. suppresses() alone can only ever answer the first two forms for a key already gone from ctx.snapshot; the id form needs the caller to have captured it earlier and offer it back through allows(). NodeLivenessTracker now carries a tracked_key_cap, mirroring LifecycleExpectationTracker's identical bound and eviction order: idle entries first (present, nothing to lose), then departed entries collapsed into one synthetic report entry that still keeps the fault raised, and a present key always wins the slot a newcomer needs. Without it, every unique identity that ever armed stayed in the tracker for the life of the process - unbounded on any fleet where nodes come and go under unique names, and a strictly larger exposure here than in the sibling this mirrors, since this detector tracks every armed App rather than an operator-declared subset. The detector may only clear GRAPH_NODE_DISAPPEARED once it has itself put a genuine FAILED on the wire at least once - not merely because report.dead was non-empty on some earlier tick. raise_fault()/clear_fault() now return whether they actually reached async_send_request(), and AggregatedFault::emit()/emit_ordered() propagate it: a report being non-empty does not mean the raise it implies was actually delivered (Advisory mode, no client, an empty source_id, the reliability gate, or a fault_manager service that is not yet ready all decline silently), and a flag set from intent rather than delivery can end up true with no FAILED ever having gone out - a kill while the service is down, followed by its own recovery, would then emit a PASSED for an occurrence the fault manager never heard about. The flag is also no longer reset on configure(): it is a fact about this process's history, not the current config, and resetting it on a live reconfigure that runs with a death still outstanding and absent left the freshly-rebuilt tracker unable to ever re-create evidence for that node, stuck without a further FAILED or a PASSED for the rest of the process's life once it genuinely returned. Also raises detectors.node_death.miss_grace to a 3000ms wall-clock floor regardless of tick cadence, and fixes the plugin's departed-lifecycle retention window to size itself off that same floored value so a cleanly shut down node's suppression survives all the way to the detector's own reclaim tick instead of expiring first and re-raising. That retention computation, and min_node_death_miss_grace() it depends on, now validate and widen before narrowing throughout: an oversized miss_grace or prune_grace value used to be narrowed to int before being range-checked, so a value like 2^32 wrapped to 0 and passed a bare ">= 0" check instead of being rejected - sizing the retention window from a number the detector itself never actually uses - and min_node_death_miss_grace() computed its own window in a 32-bit int, which overflows before the division at tick_interval_ms values the detector's own config contract already documents as valid. The integration test's own fixture rebuilt a full rclcpp node/executor/ thread pair per TEST_F (~27 cases, each running in single-digit milliseconds with no sleeps), which stalled the binary indefinitely at an arbitrary case; the shared ROS plumbing is now built once for the whole suite. Two cases represented a node's departure with a fully empty entity snapshot, which independently re-arms the reliability gate's global bringup grace and gated every raise in those cases for a reason unrelated to node_death - a live gateway's snapshot is never actually empty when one node departs, so the fix keeps a permanent unrelated entity in scope instead. test_node_death_e2e.test.py's RESPAWN_DELAY_SEC now clears the detector's own raise threshold (miss_grace+1 ticks) rather than sitting under it, so a respawning target genuinely stays absent long enough for a correct detector to confirm the outage before it returns. GRAPH_NODE_DISAPPEARED is for a node that was alive and armed; a node that was never active belongs to GRAPH_NODE_INACTIVE instead. The boundary e2e's B3 and B5 fixtures now reflect that split: both drive their target through "active" first (B3 via the same auto_activate parameter B4's target uses, B5 on every respawn, not merely the first start), since node_death only ever tracks a node reliability_allows() has armed at least once, and a managed node is never armed without reaching "active". B3 then drives a real DEACTIVATE transition before killing the node, so GRAPH_NODE_INACTIVE still matures the way the original fixture did and GRAPH_NODE_DISAPPEARED can now also join it. B5 re-arms (app_id-scoped wait_until_watchdog_armed) each respawned instance before the next kill, or a kill landing before a fresh process reaches "active" would leave that cycle's death untracked. test_node_death_e2e.test.py's ros2cli_ignored row compares tracked_count before and after three renamed-node cycles; with discovery.runtime.filter_internal_nodes off (required so those renamed nodes are visible at all), a gateway-internal hidden node used for parameter queries is visible too, and can still be inside its own warmup - and so briefly absent from tracked_count - when a single sample lands. Both samples now poll until the value holds steady for a continuous stretch instead of reading once, long enough for that node's own warmup to finish before either sample is trusted.

…le violation LifecycleExpectationTracker's absence handling advanced a required node's violation streak every tick past absence_grace, even for a streak that had not yet crossed grace when the node stopped being observed. That let GRAPH_NODE_INACTIVE mature and raise for a node entirely out of ticks gathered while nobody could see it, timestamped after the node was already gone. Split the kInactive absence branch on whether the streak is already CONTENT (is_content(), i.e. already reported under GRAPH_NODE_INACTIVE): an already-matured streak continues exactly as before, since advance_violation_streak() is a no-op past grace regardless; a streak below grace is held - neither advanced nor erased - so a node that returns still inactive resumes from where it was instead of re-earning grace from zero. count_collapsed() gets the same split, so a below-grace departed entry folded into the tracked-node-cap's collapsed count no longer fabricates a violation it never earned. Updated README.md and design/graph_watchdog.rst sentences that described the old absence-always-advances behaviour, and the detector's own grace bound: the withheld-clear guard's fixed clear cap only ever bounded a PRESENT node. The boundary e2e suite's b2 scenario now drives its target through "active" before killing it: a node that never reaches active is structurally invisible to node_death (reliability_allows() requires "active" for a managed node), so its death could never be reported no matter how the process was killed. The scenario still kills the node after observing it non-active for fewer ticks than grace - that claim is unchanged - but both of the row's claims are now reachable together, the same arming fix already applied to b3 and b5's targets.

…ycle_expectation boundary README.md and design/graph_watchdog.rst described four detectors and left GRAPH_NODE_DISAPPEARED as a reserved code with no owner. Document node_death's liveness model, its zero-config scope and exclusions, the miss_grace wall-clock floor, the allowlist/lifecycle-shutdown suppression framework and its durability contract, the tracked_node_cap eviction order (and why it can never actually saturate here, unlike the sibling detector's), the occurrence_count and freeze-frame/rosbag limits an operator reads for repeated failures, and the node_death/lifecycle_expectation boundary where both fault codes standing at once is the correct, intended state. Update the detector counts and fault-code coverage summary to match.

…vidence only NodeLivenessTracker's cap used to bound the whole tracked map: at capacity it evicted every present (idle) entry before checking whether room was made, and collapsed every non-idle entry - including one carrying only a single below-grace miss - into the monotonic collapsed count. A present/armed node could therefore be dropped from tracking entirely, and a node that had not actually crossed miss_grace could be reported dead and could never heal, since its identity was erased. The cap now bounds only the DEPARTED subset of the map (entries with a nonzero miss count). A present/armed key is admitted and re-admitted unconditionally and is never evicted to make room. Collapsing a departed entry into the synthetic count only ever applies to one that has actually crossed miss_grace; an entry still mid-grace is kept, individually tracked, until it either matures or the node returns. Also fixes a cross-slice divergence: GraphWatchdogPlugin predicts node_death's prune_grace fallback before that detector's own configure() has run, but only injected its own default when the per-detector key was absent, not when it was present but malformed. A malformed detectors.node_death.prune_grace therefore reached the detector unfiltered, which fell back to its own hardcoded default instead of the plugin's - sizing the lifecycle-departed retention window incorrectly whenever the plugin-scope default differed from that hardcoded value. Both sides now resolve through the same function. Test additions cover a graph larger than the default cap where every death is still reported, capacity pressure that no longer fabricates a death before miss_grace, a cap-forced collapse that clears once reconfigured and the graph has recovered, advisory mode's continued observation without emission, the prune_grace clamp actually reclaiming past its horizon (not merely not-yet), and the plugin/detector prune_grace fallback agreement. Doc comments that overstated fault-manager delivery guarantees or claimed every affected node is always named are corrected to what the code actually guarantees.

…sence detector could never report node_death only ever tracks an App the reliability gate has armed at least once, and the gate refuses to arm a managed node that is not active. A require_active node that comes up unconfigured and is killed before its own grace elapses is therefore invisible to node_death no matter what happens to it, and the existing absence narrowing left it invisible to lifecycle_expectation too - reported by nothing at all. Thread whether a node was ever armed into LifecycleExpectationTracker so absence may mature a below-grace violation streak only for a node the presence detector could never have reported. Where the node was armed at some point, the narrowing stands unchanged: absence still just holds the streak, since node_death owns that departure instead.

…ons in node_death prose Comments and docstrings across the node_death tests and design docs cited bare internal identifiers and an internal document by filename; each is now stated as the reasoning it stood for, inline. Docstrings written before the detector existed still described it as missing or the scenario as expected to fail; they now state what each scenario proves against the shipped detector. Comments narrating review or planning process are rewritten to state the technical finding directly, and a stale reference to another package's open, unmerged pull request is replaced with the current, verified behaviour of the fault manager it described. Also documents a known limitation: a second node's death while the first is still outstanding updates an already-confirmed record instead of maturing its own occurrence, so it gets no state transition, freeze frame, or recording of its own. The design doc records the two mitigations that were measured and rejected, and where a correct fix belongs instead. No behavioural change - source logic, test logic, and assertions are untouched.

Copilot

Pull request overview

Adds a new node_death detector to the ros2_medkit_graph_watchdog plugin to raise GRAPH_NODE_DISAPPEARED when a previously-armed node disappears and no other detector can observe/report the departure, plus an opt-in suppression framework (allowlist + clean lifecycle shutdown) and supporting tracking/retention logic.

Changes:

Introduce node_death detector with a dedicated liveness tracker keyed by stable FQN, wall-clock-based grace flooring, and bounded bookkeeping under identity churn.
Add opt-in suppression framework via a Suppressor interface, including allowlist suppression and lifecycle clean-shutdown suppression, plus pruning semantics tied to “durable” suppressors.
Expand unit/integration/e2e test coverage and strengthen e2e harness window assertions (silence/persistence/description-scoping).

Reviewed changes

Copilot reviewed 27 out of 27 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/test/test_suppressor.cpp	Adds unit tests for the `Suppressor` contract and `apply_suppressors()` behavior.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_node_liveness_tracker.cpp	Adds extensive unit tests for `NodeLivenessTracker` state machine, pruning, caps, and description ordering.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_shutdown_suppressor.cpp	Adds tests for lifecycle clean-shutdown suppression behavior against a real `ReliabilityGate`.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_expectation_tracker.cpp	Updates lifecycle expectation tracker tests to reflect the new “absence boundary” and armed-vs-never-armed split.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_lifecycle_expectation_integration.cpp	Updates integration scenarios and expectations around absence handling and boundary behavior with the new presence detector.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_graph_watchdog_plugin.cpp	Adds regression coverage for node_death retention/config fallback behavior and oversized integer validation.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/test_allowlist_suppressor.cpp	Adds unit tests for allowlist suppression matching forms and durability.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/test/e2e/harness.py	Adds stronger e2e assertion helpers for sustained presence and scoped “never names” checks; extends flaky server harness.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/graph_watchdog_plugin.cpp	Adds shared node_death prune_grace resolution and aligns retention math with detector-side validation/flooring.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/node_death_detector.cpp	Introduces the new `node_death` detector implementation (tracking, suppression, pruning, reporting/clearing guard).
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/src/detectors/lifecycle_expectation_detector.cpp	Wires reliability-gate arming into lifecycle expectation matches to support the absence-boundary split.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/suppressor.hpp	Adds the suppression interface and `apply_suppressors()` helper.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/node_liveness_tracker.hpp	Adds `NodeLivenessTracker` for armed-node disappearance tracking with cap/collapse logic and freshness ordering.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_shutdown_suppressor.hpp	Adds durable suppressor for clean lifecycle shutdown departures.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/lifecycle_expectation_tracker.hpp	Extends tracker model and absence behavior to incorporate “ever armed” logic and prevent absence-maturing where node_death should own departures.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/graph_watchdog_plugin.hpp	Exposes a test seam for departed-retention tick computation.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector.hpp	Changes `DetectorContext::{raise_fault,clear_fault}` to return `bool` to signal whether an async request was actually enqueued.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/detector_config_keys.hpp	Adds node_death grace ceiling + wall-clock miss_grace floor computation helpers.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/allowlist_suppressor.hpp	Adds allowlist suppressor implementation and matching semantics documentation.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/include/ros2_medkit_graph_watchdog/aggregated_fault.hpp	Updates aggregated fault emission helpers to return the underlying send/enqueue boolean.
src/ros2_medkit_plugins/ros2_medkit_graph_watchdog/CMakeLists.txt	Registers new unit/integration/e2e tests and adds node_death sources to relevant test targets.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Both rows raced a wall-clock assumption that only loses on a slow or contended machine. b5_restart_loop_still_caught: B5_RESPAWN_DELAY_SEC (and its mirror, RESPAWN_DELAY_SEC in test_node_death_e2e.test.py) left only 800ms of margin over node_death's own 4200ms nominal detection window - less than the graph-cache debounce ceiling alone, before ever accounting for the tick loop's own slippage under load. Losing that race does not delay a raise, it erases it: NodeLivenessTracker::update() resets a key's miss count to zero the instant it observes the node present again, with no way to recover a miss already lost that way. Both constants now carry an explicit, arithmetic-justified margin instead of a bare number. prune_no_false_heal: the reclaim-delta baseline was read the instant wait_until_watchdog_armed confirmed both nodes armed at the gate, which proves only that the RELIABILITY GATE considers them armed, not that node_death's own tick had already added their keys to tracker_. A baseline read too early undercounts, so the later tracked_count delta assertion fails for a reason that has nothing to do with prune(). The baseline is now read only once tracked_count has held steady for a sustained window, reusing the same _poll_stable_tracked_count pattern this package already relies on elsewhere for the identical hazard.

…l-clock floor miss_grace only needs to clear min_node_death_miss_grace(tick_interval_ms) - 14 ticks at the 200ms cadence these scenarios use, not an arbitrarily larger value. Dropping it from 20 to 16 ticks (3400ms nominal grace, still two ticks clear of the floor) lets the paired respawn delay shrink with it while the same 3x tick-loop-slippage factor plus the 1100ms debounce ceiling still governs the required margin: 3*3400+1100=11300ms, cleared by 12.5s with 1200ms to spare, the same relative headroom the former derivation carried at the larger grace. Applies to B5_MISS_GRACE/B5_RESPAWN_DELAY_SEC and their mirrors MISS_GRACE/RESPAWN_DELAY_SEC, cutting wall clock in the four scenarios that pay the respawn floor per cycle without loosening the safety factor.

… three Three consecutive cycles establish the claim each row makes: b5 proves the presence code catches every turn of a restart loop, not only the first; restart_loop_occurrences proves occurrence_count tracks the number of genuine deaths. A count that stops incrementing or double-counts, or a presence check that only catches the first departure, shows up by the third cycle - a fourth or fifth would repeat an already-proven claim at the full cost of the respawn delay per cycle. Renamed the two test methods that hardcoded the old cycle count in their own names.

The sanitizer test step walks the workspace one package at a time under a single 45-minute budget. ros2_medkit_graph_watchdog spends 24 minutes of it under instrumentation, so the step no longer fits: the TSan run was cut inside the package that follows, and ASan finished with 20 seconds spare. Give the package its own job, once per sanitizer, and skip it in the two workspace sweeps. The job builds only the chain up to the package, which still includes the integration-test package its scenarios launch from, and restores the matching sweep's ccache rather than saving a second copy of the same objects. Instrumentation, the timeout multiplier and the sanitizer options are the ones the sweep applied.

The restart-loop scenario runs three cycles and asserts occurrence_count 3. The package README and the design doc still described the original five. The boundary suite's large-grace comment cited a near-floor convention of 20 ticks and 4000 ms, which matches neither of the two constants it points at: those are 16 and 20 ticks, 3.4s and 4.2s nominal at the suite's tick interval.

A departure that a normal graph reports in a second or two took more than thirty under TSan, and the scenario gives up at thirty. The failure then reads as a detector that never reported, which is the same red a real defect produces. The sanitizer jobs already multiply every declared CTest timeout, and set MEDKIT_TEST_TIME_SCALE to the same factor for the budgets a test asserts internally, which that rewrite cannot reach. These files never read it. They do now, through the shared get_time_scale, for give-up bounds only: poll intervals, enforced respawn delays and the sustained-observation windows stay where they are, because stretching a window a scenario watches for silence buys no confidence and spends the package's whole test budget doing it. Unset, unparseable or below one means no scaling, so the normal jobs keep the tight budgets that give these assertions their falsifying edge.

…om the gate's silence node_death admitted a key on the reliability gate's raise permission, and that permission is deliberately permissive for a managed node whose lifecycle label has never been read: LifecycleWatcher::node_ok() must stay that way, or a node whose GetState never answers would have every fault suppressed - qos_mismatch, orphan and param_drift included. Reading permission as ownership let the presence detector latch a node it can never reliably report, and latching it also switched off lifecycle_expectation's absence path for that node, so a required managed node whose state was never measured could raise GRAPH_NODE_DISAPPEARED while GRAPH_NODE_INACTIVE stayed silent. ReliabilityGate::allows_presence_ownership() answers the stricter question - the node's lifecycle state is positively known not to be a managed-non-active one, either no managed record at all or a record reading "active" - and both detectors ask it through presence_ownership_allows(). allows_raise() and node_ok() are untouched, so no other detector changes behaviour. The latch keeps its meaning: a node that went active, deactivated and then died is still node_death's to report. The window this does not close: an App counts as managed only once the snapshot shows it advertising a GetState-typed service, so a managed node whose services have not yet been discovered is still indistinguishable from a plain one. That window is bounded by the arming warmup; the one this closes is the unbounded one. Adds test/e2e/test_presence_ownership_e2e.test.py - three scenarios against the fixture whose GetState never answers - and a gate-level sweep over every lifecycle state the watcher can report.

Withholding ownership for an unread lifecycle label was written as an absolute rule, and absolute is wrong once the ignorance can no longer resolve. A managed node whose GetState never answers, with no require_active entry naming it, was reported by nobody when it died: lifecycle_expectation returns before it looks at anything when require_active is empty, and empty is the default. That trades a racy silence for a deterministic one. LifecycleWatcher already carries the bound. It charges a GetState re-seed budget per node and charges it only for a read that actually ran, so an empty label with attempts left is "we have not finished asking" and an empty label with none left is "we asked and failed". measurement_pending() exposes that fact beside state_of(), and allows_presence_ownership() withholds only while it is true. No new configuration key; node_ok() and allows_raise() are still untouched. The same silence had a second entrance. node_death skips an app that is not online before it ever consults the gate, and the gate has no notion of online-ness, so a manifest app whose node never started was armed, carried no lifecycle record, and read as owned by a detector that would never look at it. lifecycle_expectation now ANDs the gate's answer with App::is_online at the one site that latches it. The integration fixture's app builders set is_online explicitly for the same reason: the field defaults to false, so every app in that suite was modelling a manifest entry with nothing running behind it. Also corrected: the sentence claiming the gate's ownership answer IS node_death's tracking decision. It is what that decision consults, not the whole of it - peer apps, offline apps, an empty fqn and ros2cli names are all filtered first. Tests: the gate sweep now carries the re-seed budget as a second dimension, so the empty label appears in both of its forms; a real-graph case drives the budget to zero through the watcher itself and pins the flip to the budget rather than to a tick count; status_json is asserted to serialise no-record and unread differently in one payload, which the whole e2e suite depends on; two new e2e scenarios cover the death nobody else reports, one of them in the shipped configuration with no require_active at all; and one more covers a node measured active that loses its lifecycle services before it dies.

…s own re-seed budget

…ally names an owner Waiting for the GetState budget delayed the defect this slice exists to close rather than closing it. The ~/transition_event subscription is created independently of the seed budget and is never torn down when it runs out, so a real label can still arrive after the asking stops. A node admitted because nobody had measured it would then be latched by NodeLivenessTracker, learn it was inactive all along, die, and be reported as GRAPH_NODE_DISAPPEARED - the exact boundary this slice enforces. Ownership now answers with a GROUND rather than a boolean. Earned - a state read as "active", or no lifecycle to read at all - is latched exactly as an armed key always was, because a node that ran and then deactivated is still a death when it stops running. Provisional - nothing known, nothing more to be asked - holds only while that stays true: node_death releases the key while the node is still alive the moment a label arrives and says the node was another detector's, and lifecycle_expectation latches the earned ground only. The status route now reports measurement_pending per entity. An unread label means two opposite things - the watcher is still asking, or it has stopped - and nothing else on the route separates them, so neither an operator nor a test could tell a node about to be measured from one that never will be. Four sentences the previous commit left contradicting the code are corrected: ownership is not "positively known" once an unread label is admitted, nothing "will ever measure" a node is false while its transition_event subscription stands, and waiting for the budget delays the race rather than preventing it. The design doc also regains two paragraphs an aborted edit dropped from that commit, and records what arming has always cost: a node that lives fewer than warmup_cycles ticks is owned by nobody, which is the bringup-quiesce trade-off rather than a gap in this boundary. Tests: a new e2e drives the whole sequence - unread, budget spent, a late "inactive" over transition_event, then the kill - and asserts the handover, so it is also the one scenario that rejects a detector wired to the permissive reliability_allows(); the watcher gains a two-node test pinning that exactly the read which RAN is charged, which is what the whole bound rests on; and the gate test now observes the re-seed budget at both ends instead of inferring the spend from elapsed ticks. unreadable_lifecycle_node gains a transition_label parameter, defaulting to the "active" every existing scenario depends on.

build-and-test walks the workspace under a single 45-minute cap on the test step rather than on the job, so a package that overruns is killed mid-package and reports no test name. ros2_medkit_graph_watchdog now spends 24 minutes of that step: lyrical was cut inside the package, and humble finished with three minutes to spare. Give the package its own job over the same two distros, and skip it in the sweep. The job builds only the chain up to the package, which still includes the integration-test package its scenarios launch from, and restores the sweep's ccache rather than saving a second copy of the same objects. The per-distro DDS choice is the one the sweep applies. Jazzy is untouched: it caps the job rather than the step and still has headroom.

bburda added 8 commits August 19, 2026 17:16

Copilot AI lite review requested due to automatic review settings August 19, 2026 15:49

Copilot started reviewing on behalf of bburda August 19, 2026 15:50 View session

Copilot AI reviewed Aug 19, 2026

View reviewed changes

bburda self-assigned this Aug 19, 2026

bburda added 12 commits August 19, 2026 20:38

merge: presence ownership rests on knowledge, bounded by the watcher'…

b39e477

…s own re-seed budget

merge: presence ownership yields when the graph finally names an owner

8fe4902

Navigation Menu

Uh oh!

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

Detect a node that dies when nothing else is left to report it#625
bburda wants to merge 20 commits into
mainfrom
feat/graph-watchdog-node-death

bburda commented Aug 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bburda commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Summary

Also in this change: the absence boundary

Known limitation

Also in this change: presence ownership rests on knowledge

Also in this change: the sanitizer test budget

Issue

Type

Testing

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bburda commented Aug 19, 2026 •

edited

Loading