diagnostic_bridge: keep the numbers behind a fault, and stop confirming every planned STALE by ahmedsleem109 · Pull Request #691 · selfpatch/ros2_medkit

ahmedsleem109 · 2026-09-15T22:25:50Z

What this is

Both halves of #668, which are the same complaint about the same message: a
DiagnosticStatus carries more than the bridge was willing to use.

The numbers behind the fault (#668 part 1)

A sensor fusion node publishes rejected_fixes=37, nis=0.03. The bridge read those
key-values only to pick a fault code and dropped the rest, so the fault record said a node
complained but not what it saw.

A FAILED report now carries them. ReportFault.srv gains
diagnostic_msgs/KeyValue[] evidence - the type the bridge already receives, and
ros2_medkit_msgs already depends on diagnostic_msgs, so this costs no new dependency and
no conversion. The fault manager keeps them in the fault's freeze frame under x-reported
and GET /api/v1/apps/{app}/faults/{code} serves them:

$ curl -s .../faults/FUSION_DIVERGED | jq '.environment_data.snapshots[0].data | fromjson'
{"x-reported": {"rejected_fixes": "37", "nis": "0.03"}}

A field rather than a separate attach call. The issue left this open. The evidence is
part of the event: a second call can fail on its own and leave a fault with no numbers, or
numbers with no fault. It also keeps the door open to putting evidence on near-miss entries
later without another API change - not done here, and out of scope.

Four decisions inside that are worth looking at:

Written on every FAILED report, not only at confirmation. A code that keeps approaching
confirmation without reaching it is exactly the case the near-miss series exists for, and
evidence written only at confirmation would miss it.
x-reported, not a flat merge. The frame's other keys are topic names, always fully
qualified and so always starting with /. A reserved key that cannot collide is what lets
a reader tell a value the reporter asserted from one the fault manager sampled.
The capture path had to change. snapshot_capture.cpp rebuilds the frame from the
topics it sampled and writes it whole, so evidence reported before a confirm was silently
erased by it. Which of the two writes lands last is a matter of when the fault confirmed,
not of intent, so the capture path now carries the key across. There is a test for exactly
this ordering.
The read path had to change. The frame was served only when no snapshots remained (or
retention was on), because a frame of sampled topic values duplicates the snapshots beside
it. Evidence is not in them at all, so a frame carrying evidence is now always served.

Bounded per fault code: 32 entries, 512 characters per value. Over-long values are dropped
whole rather than truncated - half a number read back later is worse than a logged absence -
and drops are reported with a throttled warning. A key already stored can still be updated,
so a steady reporter at the bound can refresh its own numbers instead of freezing at whatever
the first report said.

STALE is a decision, not a fact (#668 part 2)

map_to_severity sent STALE to CRITICAL unconditionally, and CRITICAL bypasses debounce
(fault_storage.cpp:148), so a GPS that goes STALE in every tunnel confirmed a CRITICAL
fault on its first sample - the debounce settings an operator chose never got to run.

Two parameters:

stale_severity: "WARN"                      # default CRITICAL - unconfigured behaviour is unchanged
"stale_severity_overrides.gps": "WARN"      # name or prefix, longest match wins
"stale_severity_overrides.safety_chain": "CRITICAL"

Only STALE is configurable. WARN and ERROR are facts about the status; STALE is the only
level a node reaches by design, and it is the only one that needed an operator's judgement.

Prefix matching because diagnostic names are conventionally <component>: <check>, so a
component prefix covers every check it publishes without listing them. Longest match wins,
the same rule the fault manager's entity thresholds already use - one matching rule for an
operator to learn, not two.

A typo is ignored, not defaulted. An unparseable override is reported and skipped,
leaving stale_severity in force. Applying CRITICAL to a misspelling would hand back exactly
the immediate-confirm behaviour the operator was configuring their way out of, and it would
look like the configuration had been accepted.

One pre-existing doc bug fixed

docs/config/diagnostic-bridge.rst documented STALE as SEVERITY_ERROR (2). The bridge has
always sent CRITICAL. The table now says what the code does, with a warning explaining why
the difference matters.

Tests

test_reported_evidence.cpp - 14 cases: the merge, provenance separation, later reports
updating keys, both bounds and their exact boundary, an unreadable stored frame,
"everything dropped writes no key", the capture-path preservation in both directions, and
what the read path gates on.
test_diagnostic_bridge.cpp - 10 new cases: the default, the global parameter, override
beating default, prefix and longest-prefix matching, a non-CRITICAL default combined with a
CRITICAL override, both unparseable paths, case-insensitivity, and the parser itself.
test_reported_evidence.test.py - the node end to end: the issue's own example read back
through GetFault, evidence stored while the fault is still PREFAILED, a later report
updating, a report without evidence not erasing it, and a code that never carried any
still getting no frame at all.
REQ_INTEROP_109 and REQ_INTEROP_110 added, both traced by @verifies.

Existing bridge tests were updated for the signature change: map_to_severity is a method
now that STALE resolves through configuration.

Verified locally

ROS 2 Jazzy / Ubuntu 24.04. colcon test over ros2_medkit_diagnostic_bridge,
ros2_medkit_fault_manager and ros2_medkit_fault_reporter: 54 tests, 0 failures,
every linter included. Whole workspace built (--packages-skip ros2_medkit_opcua) because
ros2_medkit_msgs changed.

Numbering starts at REQ_INTEROP_109 to leave 107/108 to #690, which is open against the
same tree.

Closes #668

bburda

Thanks for this one too :) The description explains every decision, which made the review much easier, and the STALE severity code looks good to me.

There are many comments, so a short guide. The UTF-8 crash and the missing PostgreSQL catch can take the fault manager down, so I would start with those two. The race with the capture thread, the moving timestamp and the duplicated topic values all go away if the evidence moves to its own table. I am happy to discuss that design with you before you start on it.

A few things outside this diff also need an update:

src/ros2_medkit_fault_manager/README.md (freeze-frame section and the retain_on_clear paragraph) and the capture() comment in snapshot_capture.hpp still say that a frame exists only with a capture config and is served once the snapshots are gone.
docs/api/rest.rst, Snapshot Types, describes freeze_frame as data captured at confirmation and does not mention x-reported.
src/ros2_medkit_diagnostic_bridge/design/index.rst still shows map_to_severity(level), the fixed STALE -> CRITICAL mapping and the "STALE as CRITICAL" section.
src/ros2_medkit_fault_reporter/README.md and its design doc do not list the report() overload with evidence.
src/ros2_medkit_msgs/README.md has no evidence row for ReportFault, and no note that reporters and the fault manager now have to be updated together.

Also, src/ros2_medkit_diagnostic_bridge/test/test_integration.test.py runs the real bridge with the real fault manager, but it still only checks STALE -> CRITICAL. Could you add a STALE status with an override there, against confirmation_threshold: -3, and an ERROR status whose values you read back with GetFault? A check through the gateway REST fault detail would also catch the broken docs example.

bburda · 2026-10-08T12:24:20Z

+    return frame.dump();
+  }
+  frame[kReportedEvidenceKey] = std::move(reported);
+  return frame.dump();


A key or value that is not valid UTF-8 makes dump() throw type_error.316, and nothing up to rclcpp::spin catches it, so the fault manager aborts. I reproduced it through the real bridge with the status value 80\xb0C (a Latin-1 degree sign). The bridge sends it again on the next cycle, so the node also dies after a restart. Could you use dump(-1, ' ', false, nlohmann::json::error_handler_t::replace) here and at the other two dump() calls, like fault_audit_log.cpp does, and add a launch test that sends such a status through the bridge?

Fixed in 237d92a. All three dump() calls now use error_handler_t::replace.

Doing it only in dump() was not enough: the stored key became \ufffdkey, but the next report still sent \xb0key, so the same key was added again on every cycle. Keys and values are now cleaned before they are checked and stored.

Added unit tests for both cases, and a launch test (diagnostic_bridge/test/test_stale_and_evidence.test.py) that sends 80\xb0C through the real bridge a few times and checks the fault manager still answers and exits cleanly. With the fix removed, that test fails with the same type_error.316 you saw.

bburda · 2026-10-08T12:24:20Z

+    pairs.emplace_back(kv.key, kv.value);
+  }
+
+  auto existing = storage_->get_freeze_frame(fault_code);


SnapshotCapture::capture runs on a CaptureThreadPool worker and reads and writes this same row, so the header comment about the single-threaded executor does not hold. When a report and a capture run at the same time, one write overwrites the other, and the new evidence or the fresh topic values are lost. Could you store the evidence in its own table keyed by fault code and join it with the frame in handle_get_fault? Then the two paths never write the same row and preserve_reported_evidence can go.

Agreed. Here is my plan, happy to change it:

New table reported_evidence(fault_code, source_id, key, value, reported_at_ns), primary key (fault_code, source_id, key), in all three backends.

On a FAILED report:

if the fault was CLEARED or HEALED before, delete the code's rows first (reset after clear)

then upsert each entry

an entry over a limit deletes its key, so an old value is not shown as current

nothing is written if nothing was merged

handle_get_fault adds the evidence to the frame as x-medkit-reported: { "<source_id>": { "<key>": "<value>" } }, with its own reported_at.

captured_at_ns stays the capture time.

When per-topic snapshots exist, the frame only carries x-medkit-reported.

preserve_reported_evidence and the capture-path change go away, so the capture thread and the report handler never write the same row.

Two questions:

Should reported_at be per key or per source?

Should the reset on a new occurrence also happen when retain_on_clear is on?

bburda · 2026-10-08T12:24:20Z

+  // its near-miss series an explanation, and a reporter that stops publishing after the first
+  // report would otherwise leave nothing behind.
+  if (request->event_type == ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED && !request->evidence.empty()) {
+    store_reported_evidence(request->fault_code, request->evidence, event_time);


After the rebase, this call needs the same catch (const FaultStorage::IgnorableConnectionException &) that main has around every other storage call in this handler. Without it, a lost PostgreSQL connection escapes the service callback and rclcpp::spin, and the node exits on the next FAILED report with evidence. Git merges this part without a conflict. Could you add the catch here, and also around the get_freeze_frame you moved up in handle_get_fault?

Added both in 237d92a. In handle_get_fault it returns the storage-unavailable error like the other calls. In handle_report_fault it logs a warning and continues: the fault is already stored at that point, so only the evidence is lost and the event is still published.

bburda · 2026-10-08T12:24:20Z

+    // A key already present is an update from a later report, not a new entry, so it
+    // does not count against the bound - otherwise a steady reporter would stop being
+    // able to refresh its own numbers.
+    if (!reported.contains(key) && reported.size() >= kMaxEvidenceEntries) {


No backend deletes freeze-frame rows, so the evidence survives clear_fault, healing and a restart. Old keys then show up as part of the next occurrence, and after 32 distinct keys over the life of a code every new key is dropped for good. Could you start the evidence from an empty set on the first FAILED report after the fault was cleared?

Will fix this together with the evidence table. See the proposal in the thread on fault_manager_node.cpp (the race comment).

bburda · 2026-10-08T12:24:20Z

+  frame.data = merge_reported_evidence(base, pairs, dropped);
+  // The frame's timestamp is when its contents were last established, and this write
+  // establishes some of them. A capture on a later confirm moves it again.
+  frame.captured_at_ns = event_time.nanoseconds();


Each FAILED report moves captured_at_ns, so the topic values captured at confirmation get the time of the last report. docs/api/rest.rst and the FreezeFrameData comment describe the frame as data captured at confirmation. Could you keep captured_at_ns as the capture time and give the evidence its own timestamp?

Will fix this together with the evidence table. See the proposal in the thread on fault_manager_node.cpp (the race comment).

bburda · 2026-10-08T12:24:20Z

+# the event on purpose: a separate attach call could fail on its own and leave a fault with
+# no numbers, or numbers with no fault.
+#
+# Bounded by the fault manager (count and total size) - a reporter that publishes at 10 Hz


The fault manager caps the entry count and the value length. It has no total-size cap and no cap on key length, and the value cap counts bytes while the docs say characters. Could you add a key-length cap next to the value cap, and describe the bounds here and in the docs the way the code applies them?

Done in 237d92a. Added a 128-byte key limit and renamed the value limit to kMaxEvidenceValueBytes. The srv, READMEs, docs and REQ_INTEROP_110 now say what the code does: 32 keys, 128 bytes per key and 512 bytes per value (checked after invalid UTF-8 is replaced), and no limit on total size.

bburda · 2026-10-08T12:24:21Z

+  // The frame's timestamp is when its contents were last established, and this write
+  // establishes some of them. A capture on a later confirm moves it again.
+  frame.captured_at_ns = event_time.nanoseconds();
+  storage_->store_freeze_frame(frame);


When every entry is dropped, this still writes the frame. A code with no capture config then gets a {} row that GetFault serves as an empty freeze frame, and an existing frame gets a new timestamp. Could you skip the write when nothing was merged?

Will fix this together with the evidence table. See the proposal in the thread on fault_manager_node.cpp (the race comment).

bburda · 2026-10-08T12:24:21Z

+/// The frame's other keys are ROS topic names, which are always fully qualified and so
+/// always start with '/'. This name cannot collide with one, which is what lets a reader
+/// tell a value the reporter asserted from one the fault manager sampled off a topic.
+inline constexpr const char * kReportedEvidenceKey = "x-reported";


Could you name this x-medkit-reported? All other vendor keys we put on the wire are x-medkit or x-medkit-*, and this key reaches REST clients as it is, so renaming it after a release would break them.

Renamed to x-medkit-reported everywhere (code, tests and docs) in 237d92a.

bburda · 2026-10-08T12:24:21Z

  int64_t captured_at_ns{0};
 };

+/// Key inside the freeze-frame JSON object under which reporter-supplied evidence is kept.


The FreezeFrameData comment above (lines 129-134) and the get_freeze_frame comment (lines 376-378) still say that a frame exists only for codes with a capture config and is written at confirmation. Could you update both?

Updated both in 237d92a, plus the store_freeze_frame comment and the capture() comment in snapshot_capture.hpp.

bburda · 2026-10-08T12:24:21Z

   shall take precedence over global defaults. Unspecified fields shall inherit from global
   configuration. When no entity prefix matches, global defaults shall apply.

+.. req:: Configurable STALE Severity


Our REQ_INTEROP_* list tracks SOVD interoperability, and the STALE mapping in the diagnostics bridge has no SOVD counterpart. Could you drop REQ_INTEROP_109 and its @verifies tags? REQ_INTEROP_110 can stay.

Removed REQ_INTEROP_109 and its @verifies tags in 237d92a. REQ_INTEROP_110 stays.

…nfirming every planned STALE A DiagnosticStatus carries more than the bridge was willing to use. Key-values were read only to pick a fault code and then dropped, so a node that publishes outlier counts and gate statistics - the numbers it already computed to decide it was unhappy - left a fault record saying it complained but not what it saw. A FAILED report now carries them: ReportFault.srv gains an evidence field of diagnostic_msgs/KeyValue, the type the bridge already receives, and the fault manager keeps them in the fault's freeze frame under a reserved key that cannot collide with a topic name. Written on every FAILED report rather than at confirmation, because a code that keeps approaching confirmation without reaching it is the case the near-miss series exists for. Two existing paths had to learn about the key: a confirmation capture rebuilds the frame from the topics it sampled and would have erased it, and the read path served the frame only when no snapshots remained, which would have hidden the one thing the snapshots do not contain. Bounded per fault code in entries and value length; an over-long value is dropped whole and reported, never truncated into half a number. STALE was mapped to CRITICAL unconditionally, and CRITICAL bypasses debounce, so a GPS that goes STALE in every tunnel confirmed a CRITICAL fault on its first sample and the operator's debounce settings never ran. stale_severity and stale_severity_overrides make that a decision: longest matching prefix on the status name, the same rule the fault manager's entity thresholds use. Only STALE is configurable - the other levels are facts about the status, not deployment choices. An override that does not name a severity is reported and ignored rather than defaulted, because applying CRITICAL to a typo hands back exactly the behaviour the operator was configuring their way out of. docs/config/diagnostic-bridge.rst documented STALE as SEVERITY_ERROR. The bridge has always sent CRITICAL; the table now says what the code does. Closes selfpatch#668

…erity - Replace invalid UTF-8 in evidence keys and values instead of throwing. A Latin-1 byte in a diagnostic value used to crash the fault manager. - Catch IgnorableConnectionException around the new storage calls in handle_report_fault and handle_get_fault. - Rename the evidence key to x-medkit-reported. - Add a 128-byte key limit and count the value limit in bytes. - Remove REQ_INTEROP_109 and its @verifies tags. - Add a bridge launch test at confirmation_threshold -3: STALE with and without an override, ERROR values read back with GetFault, and a non-UTF-8 value. - Update the docs and comments that still described the old behavior.

ahmedsleem109 · 2026-10-08T14:30:32Z

Thanks for the detailed review, and for reproducing the UTF-8 crash.

I rebased on main and pushed 237d92a with the fixes that don't depend on the storage design:

the UTF-8 crash
the missing PostgreSQL catches
the rename to x-medkit-reported and the key length limit
the doc and comment updates, including the files outside the diff you listed
removing REQ_INTEROP_109
a new bridge launch test, test_stale_and_evidence.test.py

The new launch test runs the real bridge with the real fault manager at confirmation_threshold: -3, in its own file so the old tests keep their timing. It checks:

STALE with an override is first PREFAILED at WARN, then confirms
STALE without an override is CRITICAL and confirms at once
ERROR values come back from GetFault
a value that is not valid UTF-8

I left out the gateway REST check for now, because the REST shape changes with the new table. I'll add it with that change.

For the evidence table I'd like to agree on the design with you first. My proposal is in the thread on fault_manager_node.cpp (the race comment). The related comments will be fixed together once we agree.

ahmedsleem109 force-pushed the feat/diagnostic-evidence-and-stale-severity branch from 5f282dc to 2745747 Compare September 15, 2026 22:41

bburda self-requested a review October 6, 2026 11:59

bburda reviewed Oct 8, 2026

View reviewed changes

ahmedsleem109 added 2 commits October 8, 2026 16:42

ahmedsleem109 force-pushed the feat/diagnostic-evidence-and-stale-severity branch from 2745747 to 237d92a Compare October 8, 2026 14:29

Conversation

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

Uh oh!

What this is

The numbers behind the fault (#668 part 1)

STALE is a decision, not a fact (#668 part 2)

One pre-existing doc bug fixed

Tests

Verified locally

Uh oh!

bburda 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!

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!

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!

ahmedsleem109 commented Oct 8, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ahmedsleem109 commented Sep 15, 2026 •

edited

Loading