Conversation
A fault record is now the pair (fault_code, reporting source). The source is the source_id a ReportFault call carried, and it owns the record. ClearFault, GetFault, GetSnapshots and GetRosbag gain a trailing request field source_id naming that owner. Empty means unscoped: the call applies only when exactly one record carries the fault_code, and fails as ambiguous when several do. On GetRosbag the field scopes the fault_code lookup only, the recording_id path is unchanged. Fault.msg and ReportFault.srv do not change shape. Their header comments, FaultEvent.msg, ListFaultsForEntity.srv and the package README are rewritten to describe the per-record model instead of aggregation by code.
A fault record is now identified by the pair (fault_code, owner), where the owner is the source_id of the ReportFault call that created it. Two sources reporting one code are two records: status, debounce counter, occurrence count, timestamps, severity, freeze frame, snapshots, near misses, rosbag links and capture cooldown are all per record, and a clear or a heal driven by one owner never touches another owner's record. Storage: FaultId addresses a record, FaultState carries its owner and emits it as the single entry of reporting_sources, and the virtual API takes a FaultId wherever it used to take a bare code. get_faults_by_code is new and backs the unscoped service resolution. check_time_based_confirmation and reclassify_healed_as_cleared return identities, because one code can hold a moved record for one owner and an untouched one for another. SQLite: faults gains owner with a composite unique index in place of the single-column primary key, freeze_frames is rebuilt the same way, snapshots and rosbag_files gain owner, and the rosbag unique index widens. A legacy database is migrated by an explicit rebuild under one transaction, probing each table on its own and backfilling the owner from the first entry of the old reporting_sources array, which is the only per-source fact the old schema recorded. Re-opening a migrated database changes nothing. Node: ClearFault, GetFault, GetSnapshots and GetRosbag resolve their target record from source_id, or unscoped when exactly one record carries the code, failing with an ambiguous: message that lists the owners otherwise. Every audit row now names the record's owner, so the clear_service, auto_heal, auto_confirm_timer and startup_reclassify literals are gone. Capture, snapshot writing and rosbag linking are per record, and correlation forms every relation between records of one owner. The unit tests that pinned one record per code are rewritten to the record contract rather than deleted, and the new cases cover two owners of one code, the unscoped resolution, the legacy-database migration and the burst that links one recording to two records.
The integration cases that reported one fault_code from several sources pinned the shared-record model: one row, one counter, one clear. They now assert what the wire actually carries. Two sources reporting one code are two records with their own severity, description and occurrence count. Debounce depth is what one reporter has said, so the cases that built a counter out of reports from several sources now report from one. A PASSED is addressed to the record its own source owns. A scoped clear takes one record and leaves the other CONFIRMED, an unscoped clear of two is refused with an ambiguous message and changes nothing, and a correlation cascade auto-clears only the symptom record of the owner whose root cause was cleared.
Every sentence that said one fault_code is one fault, that the debounce counter is shared by every source of a code, or that a fault tracks all its reporting sources is rewritten to the record model: one record per (fault_code, reporting source), each with its own state, each cleared on its own. Covers the fault manager README, its design page, the fault manager and snapshot config comments, the message reference and the fault manager configuration page, including how source_id scopes the single-record services and what an unscoped call does when several records carry the code.
…r migration Muting is a property of a record, not of a fault code. ListFaults built a set of muted codes and erased every fault carrying one, so a second owner's un-muted CONFIRMED record disappeared from the default view because the first owner's identical code was muted under their root cause. The node now filters with the engine's per-record is_muted, and muted_faults keeps its shape with one entry per muted record. GetRosbag reported fault_codes one entry per rosbag_files row, so a recording shared by two owners of one code repeated that code. The codes are deduplicated in first-seen order, which is what a caller authorizing a download needs. The owner migration no longer refuses a database over the content of one row. It ran json_extract over reporting_sources, and an earlier serializer escaped only the quote, the backslash and \b \f \n \r \t, so a source_id carrying any other control byte was written as text no JSON parser accepts. The rebuild then rolled back and the node failed to start on every restart. The owner is now recovered in C++ from the raw text, a row from which nothing can be read keeps an empty owner and one warning, and new rows escape every byte below 0x20 as \u00XX so the column is always valid JSON. The rebuild drops its scratch tables first, so debris from an interrupted run or a manual repair cannot block every later open. Evidence rows take the owner of their fault code only when that code has exactly one owner. The uncorrelated subquery this replaces picked an arbitrary owner on a shared code, which handed one owner another owner's snapshots and recordings. That backfill now runs on every open rather than only on the open that adds the column, so a store left half migrated is healed rather than left with rows no owner can see. Text the change falsified: the audit log records a healed row under the record's owner, not under a mechanism name, and an already-confirmed record updates its timestamp and severity but never its reporting sources. The ambiguity refusal is two sentences instead of a semicolon splice.
A migrated record never carries an empty owner. Where the legacy reporting_sources holds nothing readable the record gets the synthetic owner `legacy`, so it stays addressable through source_id like any other record: it can be read and cleared scoped, and it appears under that name in an ambiguity refusal. An empty owner left the record unreachable by any call, and because its evidence rows could then never be assigned to anyone while still looking like pending work, every open re-entered the migration write transaction and repeated a warning that nothing would ever resolve. The re-entry check now asks whether any child row can actually be assigned, which is the backfill's own condition, so a row nothing can resolve is left alone instead of retried forever. The empty owner now means exactly one thing, an evidence row not yet assigned to a record. Recovery of a value that is neither a JSON array nor a bare word (an object wrapper, a value behind a byte-order mark) also yields `legacy`. Returning such text whole minted an owner out of punctuation. MutedFaultInfo gains a trailing source_id. One entry is one muted record, and two owners muted on one fault code produced two entries that no reader could tell apart or address. The owner is filled from the engine's map key, so an entry cannot name a record other than the one it is filed under. Dropping a leftover migration scratch table is logged with the table named. The names faults_new and freeze_frames_new are reserved for the rebuild, and throwing away debris under them has to be visible to whoever is repairing the database. Also reworded the prose this branch added that spliced clauses with a semicolon.
Comments and a few messages in the fault manager joined two clauses with a semicolon. They are split into sentences. Two test comments opened with a bare label instead of the behaviour they pin, and one test used a vendor-specific source id. Both now say what they test in plain words.
capture_on_confirm takes the FaultId of the record that confirmed, but its Doxygen block still described a fault_code parameter. Doxygen warned twice: once for the unknown fault_code argument and once for the undocumented id. Name the parameter it actually has.
test_03 still asserted the old aggregate: two nodes reporting one fault code shared one record that listed both sources. The fault manager now keeps a record per (fault_code, reporting source), so the test failed with "'/node_b' not found in ['/node_a']". Rewrite it to the per-record contract. Two sources reporting one code give two records, each naming only its own source. Edge counting is checked per record (a re-report of an active record does not bump its count), and severity escalates only inside the record whose source sent the higher severity, so the other source's record keeps its own.
test_06 waited for one SHARED_SENSOR record carrying both forwarded hardware ids. Each hardware id is now the owner of its own record, so the wait timed out with only the camera's record in view. Wait for each hardware id's own record instead and check that the code has exactly two records, one per hardware id, both CRITICAL. The publish_until helper takes an optional owner so it can pick one record of a code that several sources report, and its failure names that owner.
The snapshot cap, the recording cap, the recapture cooldown and the near-miss series are all kept per fault record, a (fault_code, source) pair, but the configuration reference, the snapshot tutorial and several comments still gave the fault code as their unit. The near-miss section also said the debounce counter was shared by every source of a code, which is no longer true: each record has its own counter, so an entry's threshold and counter belong to the same source. Name the record as the unit everywhere these limits are described, and say that retrieval of a bag goes through its rows rather than through the fault code.
Several texts still described a fault code as one record that gathers every source reporting it: - the action status bridge said reporting_sources is append-only as the reason it never re-attributes a provisional source. The reason now is that a swapped source opens a second record and strands the first. - the roadmap listed multi-source aggregation into a single entry. - ListRosbags.srv said the entity is matched by prefix against reporting_sources, while the fault manager matches the recording's owner exactly. - the gateway README listed reporting sources among what a fault_updated event can change. A record's source never changes. - the package tables described the fault manager as fault aggregation and the recapture cooldown as per fault code. Reword each to the per-record model.
An earlier fault manager opened on a store this release migrated does not cope with it. It aborts on the first report of a code the store does not hold yet (NOT NULL constraint failed: faults.owner), and for a code two sources share it writes one source's report into the other source's row. Tell operators to keep a copy of faults.db from before the upgrade when a rollback may be needed, and to restore that copy rather than run the earlier release on the migrated file.
clang-tidy flagged lines the per-record change added. resolve_target declared the id it returns const, which stops the return from moving it (performance-no-automatic-move). Four event waits added with the per-record tests checked size() >= 1 instead of empty() (readability-container-size-empty). Drop the const and use empty().
| for (const auto & prc : pending_root_causes_) { | ||
| // A root cause only explains its OWN reporter's faults. Without this the first | ||
| // owner to report a root-cause code would mute every other owner's symptom. | ||
| if (prc.fault_id.owner != id.owner) { |
There was a problem hiding this comment.
This owner check (and the (rule_id, owner) cluster key at line 437) turns every hierarchical and cluster relation into a same-owner relation, which #698 does not ask for and which silently retires the documented cross-node behaviour: the tutorial's E-stop cascade and sensor storm span nodes, docs/tutorials/fault-correlation.rst still explains skip_correlation_auto_clear as the guard against cascading into "apps in other entities", and the fault_manager integration tests only pass because the symptom source was rewritten from /motor_node to /estop_node (test_integration.test.py:814-816) and the storm from /node{i + 1} to /node1. Keep matching by code across owners and make the relation per record (mute and cascade each symptom FaultId on its own, which is what "muting hides a record rather than a code" means), or if same-owner correlation is the intended design, call it out as a breaking change in the PR and update the tutorial.
| The rebuild is one way. A fault manager from an earlier release opened on a migrated store aborts | ||
| on the first report of a fault code the store does not hold yet | ||
| (`NOT NULL constraint failed: faults.owner`), and on a fault code two sources share it writes one | ||
| source's data into the other source's row. If a rollback may be needed, keep a copy of `faults.db` |
There was a problem hiding this comment.
"Aborts on the first report of a fault code the store does not hold yet" understates the downgrade: main's initialize_schema runs DELETE FROM rosbag_files WHERE id NOT IN (SELECT MAX(id) FROM rosbag_files GROUP BY fault_code, file_path) on open, which silently drops one owner's link row for every bag two owners of a code share (the burst case finalize_post_fault_recording now produces), before any report and before the NOT NULL abort, so an earlier release opened on a migrated store loses rows quietly rather than failing loudly. Either guard the old code path (a faults view over a renamed table makes main's startup UPDATE faults SET last_occurred_ns ... throw before it writes anything) or state here that opening a migrated file with an earlier release deletes rows and is unsupported, with the backup as the only way back.
| # Reporting source that owns the fault record (the source_id used in ReportFault). | ||
| # Together with fault_code it identifies one record. Empty: the call applies only | ||
| # when exactly one record carries fault_code, otherwise it fails as ambiguous. | ||
| string source_id |
There was a problem hiding this comment.
The in-tree OPC UA plugin still calls this service without the new field: OpcuaPlugin::send_clear_fault (src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp:1284, reached from on_alarm_change, on_event_alarm and the FaultProvider clear_fault) sends only fault_code, while its reports carry entity_id as source_id. PLC_COMMS_LOST is raised per component, so once a second emitter of that code has ever reported to the same fault_manager the plugin's clear is refused as ambiguous (cleared records count too) and, since the plugin never reads the response, the fault stays CONFIRMED with no error on the plugin side. Pass the entity id through as request->source_id at the three call sites.
| /// Pending cluster with steady_clock timestamp for window tracking | ||
| struct PendingCluster { | ||
| ClusterData data; | ||
| std::string owner; ///< Every member record of this cluster has this owner |
There was a problem hiding this comment.
Clusters are per owner now, but ClusterInfo.msg still carries bare fault_codes and no owner, so a consumer of ListFaults(include_clusters=true) can neither address the member records with the single-record services nor tell two clusters of one rule apart beyond the counter suffix in cluster_id. MutedFaultInfo got source_id for exactly this reason - give ClusterInfo the same field, filled from PendingCluster::owner.
| auto fault = storage_->get_fault(reclassified_id); | ||
| if (fault) { | ||
| audit_transition(kTransitionCleared, *fault, "startup_reclassify", reclassified_at_ns); | ||
| audit_transition(kTransitionCleared, *fault, reclassified_id.owner, reclassified_at_ns); |
There was a problem hiding this comment.
Writing the owner into source for the automatic transitions (here, the timer at line 460, the heal at 905, the cascade at 1060) drops information the chain used to carry: main wrote startup_reclassify, auto_confirm_timer, auto_heal and clear_service, and now a cleared row from an operator's ClearFault, a startup reclassification and a correlation cascade are byte-identical, as are confirmed by report and by timer, which the README's new claim that "the transition column already says what moved the row" (README.md:253) does not cover. Keep the driver in the row alongside the owner, for example a new field folded into canonicalize only when non-empty so existing chains still verify, and correct the README sentence.
This branch has not been deployed
2 participants
Summary
The fault store keyed a fault by
fault_codealone and folded every reportingsource_idinto one row withone status. Two entities raising the same code collapsed onto one record, and a clear or heal from either
ended it for both. A fault record is now identified by
(fault_code, owner), where the owner is thesource_idthe report arrived with. Status, debounce, severity, occurrence count, freeze frames, snapshots,rosbag links and audit rows are per record, and clears, heals and every per-record service call act on one
record.
This is the fault manager and message half of #698. The gateway half is #700, stacked on this branch.
This branch builds and passes the whole workspace on its own. The gateway on main still addresses faults by
code, so on this branch alone a per-entity route for a code that two sources share is refused as ambiguous until
the second PR lands.
Issue
Type
unscoped clear of a shared code is refused)
What changes
Messages and services
ClearFault.srv,GetFault.srv,GetSnapshots.srv,GetRosbag.srvgain a trailingstring source_id.Empty means unscoped, refused with a message starting
ambiguous:when several records share the code.MutedFaultInfo.msggains a trailingstring source_id.Fault.msgandReportFault.srvare unchanged.ListRosbags.srvdocuments the exact match it implements.Fault manager
(fault_code, owner)in both storage backends.reporting_sourceskeeps its shape andcarries the record's owner. Correlation mutes per record, and
ListFaultshides a muted record, not amuted code. Heal audit rows carry the owner.
GetRosbaglists a shared bag's codes once.faultsandfreeze_framesare rebuilt with a backfilled owner,snapshotsandrosbag_fileswidened, unique indexes over(fault_code, owner). The migration never aborts on a row'scontent (a legacy
reporting_sourcesthat is not valid JSON yields the ownerlegacy), never leaves adatabase unopenable (scratch tables are dropped with a logged warning), assigns child rows only when the code
has exactly one owner, and heals a partially migrated database on the next open. The serializer escapes every
control byte so new rows are always valid JSON.
the rows of a shared code. The README says to keep a copy of
faults.dbbefore upgrading.Other packages
Testing
failure. The two rewritten fault reporter and diagnostic bridge tests fail on main, as they should.
compiler warning beyond main's.
scratch tables and malformed legacy rows. Every record survives, owners come from the first reporting source.
Checklist