fix(errorgraph): attribute topic-wait roots to the blocked dependent, not the expected publisher by spurnvoj · Pull Request #120 · ctu-mrs/mrs_lib

spurnvoj · 2026-08-20T14:39:00Z

Summary

What changed:
- find_error_roots(), find_roots(), and find_dependency_roots() now attribute a missing-topic root to the component actually blocked waiting for it, via a shared append_info() helper, emitting one entry per distinct waiter instead of collapsing multiple waiters (or misattributing) into one.
- topic_info_t is split into two identity fields: source_node (who this entry is attributed to) and expected_publisher (unchanged meaning — who was expected to publish the topic). to_msg() uses each for the right output field, so the expected publisher stays correctly visible inside the error's waited_for_node.
- waited_for_nodes() now only considers genuine TYPE_WAITING_FOR_NODE errors — a waiting_for_topic error's expected_publisher metadata no longer counts as a dependency claim on that node.
- find_element_mutable(node_id_t) now only matches real node elements, never a topic placeholder that happens to share the same identity.
Why:
- A missing-topic root used to be attributed to the topic's expected publisher, even though that publisher never reported anything — the identity was fabricated at serialization time, with a timestamp that was always zero. A healthy, actively-reporting publisher could show up in root_errors as its own "root cause" purely for being named in someone else's config, while the component genuinely stuck waiting was invisible in the aggregated output.
- That same expected_publisher metadata was also feeding the graph's "even a healthy node becomes a root if something depends on it" mechanism (intentional, pre-existing design for genuine node-dependencies) — so a node could become root-eligible purely for being named, even with nothing establishing it actually caused the problem. Missing topic data can have causes unrelated to the named node's health (QoS mismatch, wrong topic name, etc.), so naming a node as expected publisher must not by itself implicate it.
- Fixing the two above removed an eager node-placeholder creation that had been (accidentally) protecting a separate, more serious bug: find_element_mutable(node_id_t) matched by identity across all element types, so once a topic placeholder existed for some identity, that same node's first real report could resolve into the placeholder instead of creating its own element — permanently losing that node's real errors (and the topic-wait entry itself) for the process lifetime, depending on the order two components happen to report in (e.g. a restarting node racing a stuck waiter).
Notes for reviewers:
- Merge order: this PR requires fix(errorgraph): fix root-finding for masked errors, dependency loops and reconverging graphs #118 to merge first. Both PRs modify the same root-determination line in find_error_roots(), find_roots(), and DFS() — fix(errorgraph): fix root-finding for masked errors, dependency loops and reconverging graphs #118 swaps the guard condition from is_waiting_for() to a new is_only_waiting_for() (a masking fix, unrelated to this PR's attribution fix); this PR restructures the body underneath to call the new append_info() helper instead of pushing to_info() directly. Rebasing this branch onto ros2 after fix(errorgraph): fix root-finding for masked errors, dependency loops and reconverging graphs #118 lands is a straightforward reconciliation (keep fix(errorgraph): fix root-finding for masked errors, dependency loops and reconverging graphs #118's guard condition, keep this PR's append_info() body), not just conflict-marker cleanup, so it's worth doing deliberately rather than blind-resolving.
- Independent of fix(errorgraph): add per-call component identity to ErrorPublisher #119 (component-identity work) — different files (error_publisher.{h,cpp} vs errorgraph.{h,cpp}), no overlap.
- The pre-existing "healthy node becomes a root because something genuinely depends on it" behavior (find_error_roots_waiting_with_no_error_dependency, from an earlier fix 5c61aeb) is deliberately untouched — only the topic-wait metadata case changes, not genuine waiting_for_node claims.
- Found and fixed incrementally via live testing in a real simulator (a misconfigured GNSS sensor topic), not just unit tests — the two follow-on bugs (field corruption, element collision) only surfaced that way.

Impact

Affected areas:
- Errorgraph implementation (include/mrs_lib/errorgraph/errorgraph.h, src/errorgraph/errorgraph.cpp)
- Tests (test/errorgraph/test.cpp) — 7 new test cases, plus a shared create_waiting_for_topic_msg() test helper and removal of one test made redundant by a new one
Breaking changes: No
- topic_info_t gained a field (expected_publisher) but existing field names/meanings are unchanged; only construction-site behavior changed, not the public message schema.

Testing status

What was tested:
- Build/test passes
- Tests updated if needed
- Tested in simulation
- Tested on real HW
How to repeat tests:

colcon build --packages-select mrs_lib --cmake-args -DENABLE_TESTS=ON
colcon test --packages-select mrs_lib
colcon test-result --test-result-base build/mrs_lib --verbose
# 410 tests, 0 errors, 0 failures, 1 skipped (pre-existing, unrelated)
# errorgraph suite specifically: 23/23 passing

… not the expected publisher find_error_roots() previously attributed a missing-topic root to the topic's expected publisher, even though that publisher never reported anything -- the message was fabricated at serialization time using the wrong identity, and its timestamp was never set (always zero). A healthy, actively-reporting publisher could show up as its own "root cause" purely for being named in someone else's waiting_for_topic error, while the component actually stuck waiting was invisible in the aggregated root_errors output. find_error_roots(), find_roots(), and find_dependency_roots() now attribute each topic-wait root to the component actually blocked on it (via a shared append_info() helper), emitting one entry per distinct waiter instead of collapsing multiple waiters (or misattributing) into one. The expected publisher is preserved as context inside the error's waited_for_node field, correctly separated from the outer identity via topic_info_t's new source_node/expected_publisher split. Separately, expected_publisher metadata on a waiting_for_topic error no longer counts as a waited_for_node-style dependency edge -- only genuine TYPE_WAITING_FOR_NODE errors do. Missing topic data can have causes unrelated to the named node's health (QoS mismatch, wrong topic name, etc.), so naming a node as expected publisher must not by itself make that node root-eligible; the pre-existing behavior for genuine node-dependencies (find_error_roots_waiting_with_no_error_dependency) is unchanged. Finally, find_element_mutable(node_id_t) now only matches real node elements, not topic placeholders that happen to share the same source_node identity (the expected publisher). Without this, removing expected_publisher from waited_for_nodes() above stopped prepare_graph() from pre-creating a node placeholder for that identity, which had been accidentally shielding a subtle bug: a publisher's first real report could resolve to a pre-existing topic placeholder instead of creating its own element, permanently losing that publisher's errors (and the topic-wait entry) for the process lifetime once triggered (e.g. by a HwApiManager restart racing a stuck sensor's topic-wait).

spurnvoj · 2026-08-20T14:40:18Z

@matemat13 sorry for the spam, but I got a nice setup when I am finding issues with errorgraph. :)

matemat13

Looks mostly good. But doesn't this break the dependency graph sometimes? Like when a node is genuinely supposed to publish a topic that other nodes are waiting for, but it itself is waiting for something else... This change would hide that dependency, no? That was the original idea why it was implemented like this...

matemat13 · 2026-09-22T20:02:20Z

          {
            waited_for_topic = msg.waited_for_topic;
            if (!msg.waited_for_node.node.empty() || !msg.waited_for_node.component.empty())
              waited_for_node = node_id_t::from_msg(msg.waited_for_node);


hmm I don't particularly like reusing the waited_for_node member for the expected publisher. Those two have slightly different semantics. I propose adding an extra field to error_t - expected_publisher and similarly to the message to make the distinction clear.

matemat13 · 2026-09-22T20:02:56Z

      topic_error.type = mrs_msgs::msg::ErrorgraphError::TYPE_WAITING_FOR_TOPIC;
      topic_error.waited_for_topic = topic_name;
-      topic_error.waited_for_node = source_node.to_msg();
+      topic_error.waited_for_node = expected_publisher.to_msg();


See my comment to erorgraph.h:R118

matemat13 · 2026-09-22T20:04:33Z

          for (const auto& el : errors)
          {
-            if (el.waited_for_node.has_value())
+            if (el.is_waiting_for_node() && el.waited_for_node.has_value())


see my comment to R118. Adding that extra member expected_publisher would also make the comment here at 251-253 redundant and this code harder to break

matemat13 · 2026-09-22T20:09:27Z

      graph_up_to_date_ = true;
    }

+    void Errorgraph::append_info(const element_t& el, std::vector<element_info_t>& out)


Can't this simply be implemented within to_info()? I.e. if the element is of topic type, it returns a vector of topic infos of the waiting nodes. If not, it returns a single-element vector of its own info. The method should then probably be renamed to to_infos() or something... I'm not particularly fond of creating an extra method within Errograph for this.

matemat13 · 2026-09-22T20:14:09Z

    {
      if (type == type_t::topic)
-        return topic_info_t{topic_name, source_node, stamp, is_not_reporting()};
+        return topic_info_t{topic_name, source_node, source_node, stamp, is_not_reporting()};


shouldn't the source_node and expected_publisher be different? isn't that the whole point?

spurnvoj requested a review from matemat13 August 20, 2026 14:39

matemat13 reviewed Sep 22, 2026

View reviewed changes

Navigation Menu

fix(errorgraph): attribute topic-wait roots to the blocked dependent, not the expected publisher - #120

fix(errorgraph): attribute topic-wait roots to the blocked dependent, not the expected publisher#120
spurnvoj wants to merge 1 commit into
ctu-mrs:ros2from
fly4future:fix/errorgraph-topic-root-misattribution

spurnvoj commented Aug 20, 2026

Uh oh!

spurnvoj commented Aug 20, 2026

Uh oh!

matemat13 left a comment

Uh oh!

matemat13 Sep 22, 2026

Uh oh!

matemat13 Sep 22, 2026

Uh oh!

matemat13 Sep 22, 2026

Uh oh!

matemat13 Sep 22, 2026

Uh oh!

matemat13 Sep 22, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

spurnvoj commented Aug 20, 2026

Summary

Impact

Testing status

Uh oh!

spurnvoj commented Aug 20, 2026

Uh oh!

matemat13 left a comment

Choose a reason for hiding this comment

Uh oh!

matemat13 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

matemat13 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

matemat13 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

matemat13 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

matemat13 Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants