fix(errorgraph): fix root-finding for masked errors, dependency loops and reconverging graphs by spurnvoj · Pull Request #118 · ctu-mrs/mrs_lib

spurnvoj · 2026-08-18T06:48:04Z

Summary

What changed:
- Masked errors: root-finding excluded any element with a waiting_for entry, even if it also had a genuine error. New is_only_waiting_for() fixes this in all three root-finding functions.
- write_dot() topic age: topics never get a stamp, so they showed a bogus age. That line is now skipped for topics.
- Reconvergence in DFS(): a diamond-shaped graph was reported as a loop. Loops are now only reported for true back-edges (on_stack).
- Pure loops: find_roots()/find_error_roots() now report loop members (loop_root), and find_dependency_roots() returns every member, not just one.
- Docs/readability: review cleanup, no behavior change.
Why:
- Masked errors: DiagnosticsManager plugins share one ErrorPublisher identity, so a real error (e.g. GNSS init failure) disappeared from /uav1/root_errors whenever a sibling reported waiting_for_topic.
- write_dot() topic age: misleading DOT output.
- Reconvergence in DFS(): false loop_detected and duplicate roots.
- Pure loops: a dependency deadlock was reported as "no errors".

Impact

Affected areas:
- Errorgraph (include/mrs_lib/errorgraph/errorgraph.h, src/errorgraph/errorgraph.cpp, test/errorgraph/test.cpp)
Breaking changes: No
- Root-finding may now return more elements (masked errors, loop members).

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=1
colcon test --packages-select mrs_lib --ctest-args -R errorgraph
# 26 tests pass; simulation check covered the masked-error fix only

spurnvoj · 2026-08-18T06:48:33Z

@matemat13 this is your child, so please check it and let me know if those changes make sense for you :)

matemat13

looks good to me from perspective of functionality, but the documentation needs clearing up. There is the question of how to handle loops in find_roots() and find_error_roots() as rn it's inconsistent with find_dependency_roots() if I'm not mistaken

matemat13 · 2026-09-22T19:10:40Z

+          return std::all_of(std::begin(errors), std::end(errors), [](const auto& error) { return error.is_waiting_for() || error.is_no_error(); })
+                 && std::any_of(std::begin(errors), std::end(errors), [](const auto& error) { return error.is_waiting_for(); });


I'd split these two lines to two named booleans to make it clearer what each one of them actually checks for. Something like

// true iff there are no "raw" errors generated directly by this node const bool no_raw_errors = std::all_of(std::begin(errors), std::end(errors), [](const auto& error) { return error.is_waiting_for() || error.is_no_error(); }); // true iff there is at least one waiting_for error (to rule out a node with all errors being no_error) const bool at_least_one_waiting_for = std::any_of(std::begin(errors), std::end(errors), [](const auto& error) { return error.is_waiting_for(); }); return no_raw_errors && at_least_one_waiting_for;

Although it avoids the early return of the && if the first part of the condition is met, this is only relevant for non-optimized builds, and it's worth the readability improvement IMO.

Done in d323633, using your suggested names no_raw_errors / at_least_one_waiting_for.

matemat13 · 2026-09-22T19:14:11Z


      void build_graph();

      std::vector<const element_t*> DFS(element_t* from, bool* loop_detected_out = nullptr);


may be worth documenting even though it is private... it's a bit unclear what the returned value actually is. It's interpretation as the "roots" seems to be a bit different than that one in find_error_roots() and find_roots(), so it's worth clearing up IMO.

Documented in 7f089d8. The doc block now explains what the returned "roots" are: every visited element with a genuine error, plus every member of a genuine dependency loop. It also explains how that feeds find_dependency_roots() directly, and find_roots()/find_error_roots() through element_t::loop_root. There's also a note on how a real back-edge is told apart from a diamond-shaped graph where paths rejoin. That was a separate bug, fixed in 4102868: it used to be reported as a loop.

matemat13 · 2026-09-22T19:17:41Z

      std::vector<element_info_t> find_dependency_roots(const node_id_t& node_id, bool* loop_detected_out = nullptr);

      /**
       * \brief Find all root-cause elements across the entire graph.


I'd update the documentation to make it clear how it's different from find_roots().

also, how does this handle loops? I don't remember and it's not documented. If I'm reading the code correctly, loops are not reported by this method.

Clarified in the doc. find_error_roots() is find_roots() minus healthy (no-error, actively reporting) elements that nothing waits on, so uninteresting leaves aren't reported as root causes.

Also, you were right, pure loops disappeared completely. Fixed in 7f089d8. DFS() now marks every member of a genuine loop with loop_root, and find_error_roots() includes those members. Covered by find_error_roots_pure_loop and find_error_roots_pure_loop_with_no_error_entry tests now.

matemat13 · 2026-09-22T19:21:36Z

+        // an element is a root if it doesn't depend on anything else, or if it carries a genuine
+        // error of its own even while also waiting for something else (that error would otherwise
+        // be silently skipped as the DFS walks past it toward its dependency)
+        if (!cur_elem->is_only_waiting_for())


This comment seems to contradict R62, which also adds an element as a root if it's within a loop, which is not mentioned here. I'd add a mention here that that's handled later.

Updated. The comment now says that loop members are added as roots further down, when the back-edge is found.

matemat13 · 2026-09-22T19:24:00Z

+       *
       * \return  Copies of root element info as type-safe variants.
       */
      std::vector<element_info_t> find_roots();


Similar as the comment to R351 - I don't think this method handles loops, right? So it's behavior is different than that of find_dependency_roots(). Worth at least documenting, maybe even updating the implementation to handle that somehow...

Same fix as for find_error_roots() (7f089d8). find_roots() now includes every loop member, which makes it consistent with find_dependency_roots(). Covered by find_roots_pure_loop test. In cb62198 I also tightened find_dependency_roots_detects_loop to require every member (it used to accept any non-empty result) and added a 3-element loop test.

matemat13 · 2026-09-22T19:25:22Z

      for (const auto& el_ptr : elements_)
      {
-        // A leaf has no children (no one waits for it)
+        // A leaf has no parents, i.e. no one else in the graph waits for it


…aiting-for entry find_error_roots(), find_roots(), and find_dependency_roots()/DFS() each excluded an element the moment it had any "waiting for" entry, even when that same element also carried a genuine error. DiagnosticsManager and its sensor-handler plugins share one ErrorPublisher identity, so a real, persistent plugin error could be silently dropped from the reported root-error set whenever a sibling plugin happened to report a normal startup "waiting for topic" at the same time. Add element_t::is_only_waiting_for(), true only when every non-"no error" entry is a "waiting for" dependency, and use it in place of the any-based is_waiting_for() check in all three root-finding functions. Also correct the docstrings/comments that described the wrong graph field or an error-content filter that didn't actually exist.

…_dot() Topic elements never go through add_element_from_msg() (only node elements do), so their stamp stays at its constructor default of 0 forever, and is_not_reporting() is hardcoded false for topics. write_dot() rendered this as "age: <seconds since epoch>s" for every topic vertex regardless of whether its expected publisher ever showed up. Skip the age/not-reporting line for topic elements instead.

The combined all_of/any_of expression made it unclear what each half actually checked. Split it into two named booleans, no_raw_errors and at_least_one_waiting_for, per review feedback on PR ctu-mrs#118. No behavior change.

…loop in DFS() DFS()'s cycle detection only tracked a flat visited/unvisited flag, so reaching an element that had already been fully processed via some other, unrelated path -- e.g. a diamond-shaped dependency graph, or simply a later top-level walk in build_graph() reaching into an element an earlier one already finished -- was indistinguishable from a genuine back-edge to an ancestor on the current path. Both were reported as a loop. Track a third state (on_stack) so a loop is only reported when the revisited element is still an ancestor on the current DFS path. find_dependency_roots() no longer reports loop_detected = true or duplicates a root for an acyclic, reconverging graph.

…endency loops too Neither function had any notion of cycles, so a pure dependency loop with no genuine error anywhere in it was silently excluded entirely, unlike find_dependency_roots(). find_error_roots() is the function actually used in production, so a real stuck-dependency deadlock would report as "no errors". Mark every element on a detected cycle (loop_root) and include it in both functions' results, now that DFS()'s cycle detection is correct.

… loop member The 2-element loop test only checked that some root was returned. Require both members explicitly, add a 3-element loop regression test to confirm the loop marking works beyond the 2-element case, and document the one-root-per-member behavior in the header. No behavior change.

spurnvoj · 2026-09-23T11:46:54Z

@matemat13 thanks! I think I've covered everything:

Named booleans in is_only_waiting_for() (d323633)
New bug resolved: DFS() no longer reports a diamond-shaped graph, where paths rejoin, as a loop (4102868)
find_roots()/find_error_roots() now report pure loops, consistent with find_dependency_roots(), and the docs are cleaned up (7f089d8)
Loop tests tightened, plus a 3-element loop test (cb62198)

spurnvoj requested a review from matemat13 August 18, 2026 06:48

spurnvoj force-pushed the fix/errorgraph-root-finding-persistent-error branch from eb240db to 73e27fc Compare August 18, 2026 07:11

This was referenced Aug 20, 2026

fix(errorgraph): add per-call component identity to ErrorPublisher #119

Open

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

Open

matemat13 reviewed Sep 22, 2026

View reviewed changes

spurnvoj added 2 commits September 23, 2026 07:48

spurnvoj force-pushed the fix/errorgraph-root-finding-persistent-error branch from 73e27fc to f552396 Compare September 23, 2026 06:20

spurnvoj added 4 commits September 23, 2026 08:22

spurnvoj requested a review from matemat13 September 23, 2026 11:49

spurnvoj changed the title ~~fix(errorgraph): don't drop a genuine error masked by a co-existing waiting-for entry~~ fix(errorgraph): fix root-finding for masked errors, dependency loops and reconverging graphs Sep 23, 2026

		return std::all_of(std::begin(errors), std::end(errors), [](const auto& error) { return error.is_waiting_for() \|\| error.is_no_error(); })
		&& std::any_of(std::begin(errors), std::end(errors), [](const auto& error) { return error.is_waiting_for(); });


		void build_graph();

		std::vector<const element_t> DFS(element_t from, bool* loop_detected_out = nullptr);

Conversation

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

Uh oh!

Summary

Impact

Testing status

Uh oh!

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

Uh oh!

Uh oh!

matemat13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spurnvoj Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spurnvoj commented Sep 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

spurnvoj commented Aug 18, 2026 •

edited

Loading

spurnvoj commented Aug 18, 2026 •

edited

Loading

spurnvoj Sep 23, 2026 •

edited

Loading