fix(fault_manager): keep the near-miss series when a fault is cleared - #629
Open
bburda wants to merge 8 commits into
Open
fix(fault_manager): keep the near-miss series when a fault is cleared#629bburda wants to merge 8 commits into
bburda wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes fault-manager data loss by introducing an append-only “near-miss” series that records each FAILED report that advances debounce without confirming, and ensures the series is retained across clear_fault and startup HEALED reclassification. It implements the feature consistently across SQLite and in-memory storage backends, adds a new retention parameter (near_miss.max_per_fault, default 200), and provides comprehensive unit tests plus documentation updates.
Changes:
- Add near-miss storage (new
near_missestable in SQLite; in-memory series) and retrieval API (FaultStorage::get_near_misses). - Add retention control (
set_max_near_misses_per_fault) with oldest-first eviction, configured vianear_miss.max_per_fault. - Add extensive tests for both backends and document the new behavior and configuration.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp | Creates near_misses table, appends near-miss rows on qualifying FAILED reports, trims per-fault retention, and preserves near-miss rows on clear/reclassify paths. |
| src/ros2_medkit_fault_manager/src/fault_storage.cpp | Implements in-memory near-miss recording, retention eviction, and retrieval; adds shared is_near_miss() helper. |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp | Extends storage API with NearMissRecord, get_near_misses(), and retention setter; updates clear_fault contract docs. |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp | Declares near-miss API overrides and adds storage member for retention bound. |
| src/ros2_medkit_fault_manager/src/fault_manager_node.cpp | Declares/applies near_miss.max_per_fault parameter and clamps negative values to the default. |
| src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp | Adds SQLite near-miss series tests (append-only, not counting confirming/critical/PASSED, survives clear/reopen/reclassify, retention bounds, schema creation). |
| src/ros2_medkit_fault_manager/test/test_fault_manager.cpp | Adds node-parameter tests for near-miss retention and in-memory backend near-miss contract tests. |
| src/ros2_medkit_fault_manager/README.md | Documents near-miss semantics, retention, and persistence notes (no REST/service surface yet). |
| docs/config/fault-manager.rst | Adds configuration documentation for near-miss retention parameter and behavior. |
| src/ros2_medkit_fault_manager/config/fault_manager.yaml | Adds commented default config entry and explanation for near_miss.max_per_fault. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A FAILED report that moved the debounce counter without confirming the fault left no trace beyond the counter itself, and the counter is updated in place. Each near miss therefore overwrote the last, so nothing recorded how often a fault code approached confirmation. Append one row per such report to a new near_misses table (the in-memory backend keeps the equivalent per-code series), holding the timestamp, the counter value after the report, the confirmation threshold it was measured against, the severity and the reporting source. PASSED reports move the counter in the healing direction and are not near misses. The series is left alone by clear_fault and by the startup reclassification of HEALED faults: acknowledging one fault cycle must not erase a record that spans cycles and cannot be reconstructed afterwards. Per-topic snapshots keep their existing clear-on-acknowledge behaviour, since they belong to the single confirmed occurrence. Retention is bounded per fault code by the new near_miss.max_per_fault parameter (default 200, 0 = unlimited), evicting the oldest entries first - deliberately the opposite of the snapshot limit's keep-earliest rule, because a series frozen at boot says nothing about whether the rate is changing. A fault database written by an earlier build gains the table on first open.
…on and atomicity Three defects in the near-miss series. The SQLite backend evicted by event timestamp while the in-memory backend evicted by arrival. Reporters carry their own clocks, so a report can arrive with a timestamp behind one already stored. SQLite then deleted the row it had just written, and the two backends returned different histories for the same input. Both now keep and evict in arrival order. Setting the retention bound did not trim what was already stored. A database that grew under a larger bound, or none, stayed over the new bound until each fault code happened to record another near miss, and a code that went quiet kept its rows for good. Applying the bound now trims immediately, per fault code. The fault row write and the near-miss append were separate autocommit statements. A failure on the append left the debounce counter already advanced, so the caller's retry advanced it a second time while the near miss it retried for stayed missing. They now commit together. Two documentation statements still said snapshots are always deleted when a fault is cleared, which stopped being true when the retention switch was added.
…ear misses The near_misses index was still on (fault_code, occurred_at_ns, id) after the series moved to arrival order, so neither the read nor the trim could use it. It is now (fault_code, id). set_max_near_misses_per_fault(SIZE_MAX) bound to int64 as -1, and every row then compared as beyond the bound, so the idiomatic spelling of "no limit" emptied the table. Any bound past what SQLite can express now means unlimited, on the per-report trim as well. Applying a bound trims what is already stored, which deletes history that cannot be recovered. A mistyped parameter did that at boot with nothing said. The setter now returns how many entries it evicted and the node warns. report_fault_event took BEGIN IMMEDIATE for every report, including PASSED reports that write nothing. That made a heal heartbeat contend for the writer lock and fail with SQLITE_BUSY where before it could not. Only FAILED reports, the only ones that can write a second row, take the transaction now. InMemoryFaultStorage::reclassify_healed_as_cleared never dropped snapshots, while the SQLite backend did, so the two answered a snapshot query differently for the same calls. It now follows the same rule and the same retain switch.
The HEALED latch holds the status the whole way from the healing threshold down to the confirmation threshold. Every FAILED report on the way back into a fault that does confirm therefore moves the counter without confirming, which is the definition of a near miss, and lands in the series next to approaches that receded. Nothing in the entry told the two apart, so the series could not answer how often a code approached confirmation without becoming a fault, and under a bound the ramps evicted the approaches. Each entry now records the fault status the report left behind. PREFAILED is an approach from a resting state, HEALED is a counter walking back down under the latch. It is never CONFIRMED, since that is what excludes a report from the series. Rows written before the column existed read it as empty. Also state in the interface and the docs that with per-entity overrides the recorded confirmation threshold is the reporting source's, while the counter is shared by every source of that fault code, so it is not on its own the distance to confirmation.
…ing config's band The SQLite backend brings a stored counter back into range before applying a report; the in-memory one did not. Per-entity threshold overrides mean two sources of the same fault code are evaluated against different bands, so a counter clamped to one source's ceiling can sit above another's, and clamp(clamp(x) - 1) is not clamp(x - 1) once it does. Driving a code to a wide source's healing ceiling and then reporting FAILED from a narrower source recorded counter 2 in SQLite and 3 in memory, offsetting the whole series and the report at which the fault confirms for the rest of its life. The in-memory backend now clamps the same way, and a test drives one sequence through both backends and compares what they store.
…me visible Two defects in how the node builds a snapshot response, both reachable once snapshots outlive the acknowledgement. get_snapshots writes one entry per topic and let the last row processed win. A topic can carry several snapshots, from re-confirmations within a cycle and now from every retained occurrence, and the backends return them in opposite orders: SQLite newest first, memory oldest first. So SQLite served the OLDEST value for a topic while reporting the newest captured_at above it, and the two backends answered the same query differently. The newest capture per topic is now tracked explicitly. get_fault served the freeze-frame only when no snapshots remained, which was the signal that acknowledgement had removed them. With retention on they never run out, so the frame - the state at the most recent confirmation - stayed hidden behind snapshots of earlier occurrences. It is served in that case too.
…and the scope of the bound
…ication clear_fault is not the only place that takes a fault's readings. When healing is disabled, startup reclassifies HEALED faults as CLEARED and deletes their snapshots along the way, and that path ignored snapshots.retain_on_clear. The setting therefore held only until the next restart, which then deleted exactly what it was set to keep, and the two storage backends disagreed: the in-memory one kept the snapshots while SQLite dropped them.
bburda
force-pushed
the
fix/retain-near-miss-history
branch
from
August 21, 2026 06:35
3ad204e to
d62f66f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Nothing in the fault manager recorded how often a fault code nearly confirmed. Two behaviours
combined to destroy that information. The debounce counter was updated in place, so every report
overwrote the previous state and no series was ever built. And
clear_faultdeleted what had beencaptured for the fault, so acknowledging the fault removed the rest.
This is data being destroyed, not a feature that was missing. Any later analysis of recurring
faults, for example how often a code approaches confirmation and whether that rate is growing,
needs this series, and it cannot be rebuilt after the fact. Every day a deployment runs without
it, more of that history is gone for good. That is why this is a fix and not an enhancement.
What a near miss is
A FAILED report that moves the debounce counter but does not confirm the fault. A PASSED report
moves the counter in the healing direction, so it is not a near miss. The predicate lives in one
place,
is_near_miss(), so both storage backends agree.The near-miss series
near_missestable, one row appended per near miss, never updated in place. Each row holdsthe timestamp, the counter after the report, the confirmation threshold it was measured
against, the severity, the reporting source, and the fault status the report left behind.
resulting_statusis what makes the series readable. The HEALED latch holds the status thewhole way from the healing threshold down to the confirmation threshold, so every report on the
way back into a fault that does confirm also moves the counter without confirming. Entries
recording
PREFAILEDare approaches from a resting state; entries recordingHEALEDare acounter walking back down under the latch. Without the field the two are indistinguishable and
the question the series exists for is not answerable.
clocks, so a report can arrive with a timestamp behind one already stored; ordering by timestamp
would let such a report evict itself the moment it was written.
near_miss.max_per_fault, default 200, 0 means unlimited, evicting the oldest entries.That is the opposite of
snapshots.max_per_fault, which keeps the earliest, because a seriesthat stops growing at boot cannot show whether the rate is changing. Applying the bound also
trims what is already stored, and the node warns with the number of entries that dropped,
because a mistyped value deletes history that cannot be recovered.
clear_faultno longer touches the series, and neither does the startup reclassification ofHEALED faults, which is the second place that removed captured data.
the debounce counter advanced with the near miss missing. Only FAILED reports take that
transaction: a PASSED report writes at most one row, and taking the writer lock for it would
make a heal heartbeat fail with
SQLITE_BUSYwhere before it could not.InMemoryFaultStoragekeeps the same series, and a test drives one report sequence through bothbackends and compares what they store.
Snapshot retention, now that the switch exists on main
snapshots.retain_on_clearlanded separately, so this branch no longer adds it. What it does addis the handling that switch still needs.
setting. Retention therefore held only until the next restart, which then removed exactly what
it was set to keep, and the backends disagreed: the in-memory one kept the snapshots, SQLite
dropped them. Both follow the switch now.
~/get_faultserved the freeze-frame only when no snapshots remained, which was the signalthat acknowledgement had removed them. With retention on they never run out, so the frame - the
state at the most recent confirmation - stayed hidden behind snapshots of earlier occurrences.
~/get_snapshotsreturns one entry per topic and now serves the newest capture of thattopic. It let the last row processed win, and the backends return these in opposite orders, so
SQLite served the oldest value under the newest
captured_at.Two other corrections along the way
The SQLite backend brings a stored debounce counter back into the reporting config's band before
applying a report and the in-memory one did not. Per-entity threshold overrides make that
reachable in one process, and it offset the whole series and the report at which a fault confirms.
Before enabling anything
With the default
confirmation_threshold: -1the first FAILED report confirms the fault at once,so there is never a near miss to store. The series only fills where debounce is configured.
With
snapshots.retain_on_clearon, readsnapshots.max_per_faultas a cap for the whole life ofthe database rather than for one fault cycle. It rejects new snapshots and keeps the earliest, and
the rejection is silent, so a code that has reached the cap records nothing on later occurrences.
Raise it, or set it to 0, if you need snapshots from every occurrence.
The near-miss bound is per fault code, not per database. Fault codes are unbounded in cardinality,
so the bound caps what any single code costs, not the total.
Known limitation
There is no service or REST surface for the series. It is read through
FaultStorageor from thedatabase file, so nothing can query it operationally yet and no end-to-end test can exercise it.
Adding one needs a new service definition, which is deliberately out of scope here.
Issue
Type
Testing
Full package suite: 739 tests, 0 errors, 0 failures (unit, integration and linters).
Near-miss series, SQLite: appended and not overwritten, the confirming report is not counted,
survives
clear_fault, continues across a reactivation, survives closing and reopening thedatabase, a report after reopen extends the series in order, a PASSED report is not counted, a
CRITICAL immediate confirm is not counted,
confirmation_threshold: -1records nothing, a FAILEDreport under the HEALED latch is counted and is distinguishable from an approach, each entry
carries its own threshold and severity and source and status, arrival order wins over timestamp
order, the bound keeps the newest, a bound of 1 works, the bound is per fault code, 0 is unlimited,
SIZE_MAXis unlimited rather than emptying the table, applying a smaller bound trims and reportshow many it dropped, applying an unlimited bound keeps everything, an unknown code returns empty,
a PASSED report on an unknown fault writes nothing, the table and the status column are both
created on a database from an older build, and the series survives HEALED reclassification.
The in-memory backend has the same contract covered, and one parity test drives an identical
sequence with mixed per-entity thresholds through both backends and compares the stored series.
Snapshot retention through the startup reclassification: kept when configured, dropped by default,
unrelated faults untouched, and the same contract on both backends.
Read path, driven through the real
~/get_snapshotsand~/get_faultservices: the newestsnapshot of a topic wins, and the freeze-frame stays visible behind retained snapshots.
Node level:
near_miss.max_per_faultreaches storage, the default is 200, a negative value fallsback to the default instead of becoming
SIZE_MAX, 0 gives unlimited, snapshots are deleted onclear by default,
snapshots.retain_on_clearreaches storage, and it is applied before the startupreclassification rather than after it.
Every behaviour above was checked by mutation: reverting each fix in turn fails only the tests that
encode it, and flipping either default fails exactly the default-behaviour tests.
Checklist
behaviour, and the schema changes are additive with a migration)
docs/config/fault-manager.rst,docs/tutorials/snapshots.rst, default parameters file)