Stop PASSED events re-dating a fault, and count real SSE loss - #573
Conversation
A PASSED event set last_occurred to the clear instant, so a fault that had not occurred for hours read as if it had just happened. Only FAILED events are occurrences; the PASSED instant already lives in last_passed. Also publish EVENT_CLEARED on the CONFIRMED -> HEALED transition. The heal was audited but never reached the event stream, so consumers kept showing a fault that was over.
The drop counter incremented for every event past the 100th regardless of clients, so a box with zero SSE clients logged 19k "events dropped (slow or disconnected clients)". Eviction now tracks per-client cursors: entries all live clients already received go first, then entries superseded by a newer event for the same fault code, so a lagging client still converges on the current state of every fault. Only a genuine loss is counted and logged. Clients with no write progress for 3 keepalive intervals are reaped as dead and excluded from the backpressure accounting. Pending frames are formatted under the queue lock and written outside it, since coalescing erases from the middle of the deque.
A single lost fault transition matters; waiting for ten before the first WARN hid it.
There was a problem hiding this comment.
🟡 Not ready to approve
The semantics of fault_cleared changed but the published API docs (docs/api/messages.rst) remain inconsistent and should be updated before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes two production issues in the fault pipeline: (1) PASSED events incorrectly “re-dated” faults by advancing last_occurred, and (2) the SSE fault stream’s “dropped events” metric/logging counted buffer rotation rather than real client-visible loss. It updates fault storage semantics, ensures HEALED transitions are observable to SSE consumers, and makes SSE eviction cursor-aware so only truly owed-and-lost events are counted as drops.
Changes:
- Update fault storage backends so
last_occurredis written only on FAILED events; PASSED updateslast_passedonly. - Publish
EVENT_CLEAREDwhen a fault transitions toHEALEDso stream consumers see the fault end. - Rework SSE replay-buffer eviction to be client-cursor-aware (delivered watermark + coalescing superseded entries + reaping stalled clients), and add targeted unit tests.
File summaries
| File | Description |
|---|---|
| src/ros2_medkit_msgs/msg/FaultEvent.msg | Clarifies EVENT_CLEARED semantics to include both manual clear and auto-heal (status disambiguates). |
| src/ros2_medkit_gateway/test/test_sse_fault_handler.cpp | Adds unit tests covering “no clients ≠ drop”, state-convergent coalescing, and stalled-client reaping. |
| src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp | Implements cursor-aware eviction, coalescing, and stalled-client reaping; adjusts drop logging to count only real loss. |
| src/ros2_medkit_gateway/README.md | Updates SSE documentation for buffer eviction semantics and fault_cleared meaning. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/sse_fault_handler.hpp | Documents new SSE eviction/backpressure model and adds test/metrics accessors. |
| src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp | Adds regression test ensuring PASSED does not advance last_occurred in SQLite backend. |
| src/ros2_medkit_fault_manager/test/test_fault_manager.cpp | Adds regression tests for last_occurred behavior and for HEALED → EVENT_CLEARED publishing. |
| src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp | Stops updating last_occurred_ns on PASSED updates; keeps PASSED timestamp in last_passed_ns. |
| src/ros2_medkit_fault_manager/src/fault_storage.cpp | Stops updating last_occurred on PASSED updates in the in-memory backend. |
| src/ros2_medkit_fault_manager/src/fault_manager_node.cpp | Publishes EVENT_CLEARED when a fault transitions to HEALED so SSE consumers observe the end-state. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
bburda
left a comment
bburda
left a comment
There was a problem hiding this comment.
A few things outside this diff that I think should move with it.
Five places still describe the old contract:
src/ros2_medkit_msgs/msg/Fault.msgline 38 sayslast_occurredis the time the fault was "last reported (FAILED or PASSED event)". That is the wire contract for the field you changed, in the same package whoseFaultEvent.msgthis PR does edit.src/ros2_medkit_msgs/README.mdline 25 repeats it, and line 71 still reads "EVENT_CLEARED | Fault transitions to CLEARED".docs/api/messages.rstline 34 repeats it too, and line 131 still saysfault_clearedmeans cleared via the ClearFault service. This one is published to the docs site.- The Postman collection has the same sentence for
fault_cleared("Fault was manually cleared").
One consumer in an untouched file needs the same status check: src/ros2_medkit_gateway/src/trigger_fault_subscriber.cpp lines 69 to 73 map fault_cleared to ChangeType::DELETED without reading fault.status, so an auto-heal now raises a fault trigger with delete semantics. On the shipped bringup config healing is on, so this is the common path rather than an edge case.
On coverage: neither fix is proven end to end. test_sse.test.py launches with fault_manager=False, so no integration test has ever seen a fault frame on /faults/stream. Worth noting that the graph provider failures you mention are a different handler (cyclic subscriptions, no replay buffer and no cursors), so they do not explain this gap. The heal fix has the better evidence, since its test drives a real node over a real service and topic, but nothing carries it through the gateway to an HTTP client.
Last one is a design call rather than a defect. last_passed_ns is storage only. It is not in Fault.msg, not in fault_to_json and not in any DTO, so a consumer that used last_occurred as "when did this fault last report anything" has lost that with no replacement on the wire. Do you want to expose last_passed, or record the loss in the release notes?
Only fault_updated entries coalesce for free; a superseded transition is evicted before a code's only event but counted and logged as a real drop, attributed to clients behind the highest lost id. Last-Event-ID is digits only and clamped to the newest issued id, the watermark is an optional with no aliasable sentinel, and the unreachable stall reaping is removed (no write timeout override, so cpp-httplib fails dead sockets in 5s).
An event enqueued between flush and wait lost its notify_all and waited out a full keepalive interval, 30s in production. The predicate form of wait_for checks the queue before it sleeps.
The unregister deleter locks handler state and runs whenever the framework destroys a content provider, on any thread. The destructor now waits for clients_ to empty, so a closure outliving the handler fails loudly instead of dereferencing a dead object.
The eviction exemption protects the cascade payload, but format_sse_event never serialised it, so stream consumers could not learn about the cascade at all. Emit the field when non-empty.
Databases written before the PASSED fix keep the wrong timestamp forever on a latched fault that only ever heals. Backfill last_occurred_ns from last_failed_ns when opening the store.
last_occurred no longer moves on PASSED, which silently removed last activity from the wire. Carry the last PASSED instant through Fault.msg, both storage backends and the fault JSON (absent when never passed).
msgs README, messages.rst and the Postman collection still described the old fault_cleared and last_occurred contract. The gateway README now also covers the real-loss eviction branch, id holes after coalescing and the auto_cleared_codes payload; the trigger subscriber states why heal keeps delete semantics.
test_sse.test.py launches without the fault manager, so no integration test had ever seen a fault frame on the stream. Drive FAILED then two PASSED through ReportFault and assert fault_confirmed and fault_cleared with status HEALED arrive at a real HTTP client.
The events topic is reliable but volatile, so a confirm published moments after the fault manager starts is lost when the gateway's subscription has not matched yet - ASan CI hit exactly that and saw an empty stream. Report a sacrificial fault until its frame arrives, then run the real scenario.
Two defects found on a live diagnostic box, plus the fix for each.
A stale fault reads as freshly active
PLC_COMMS_LOSTon the Beckhoff ADS instance satCONFIRMEDfor hours while the link wasdemonstrably alive, and its
last_occurredmatched the plugin's clear log to the millisecond.The
CONFIRMEDlatch itself is not a bug:compute_debounce_statusonly returnsHEALEDwhenhealing is enabled, the box runs
healing=disabled, and that is the documented default. What is abug is that PASSED events advanced
last_occurred, so a fault cleared three hours ago presented ascurrent. The clear instant already has its own field,
last_passed_ns;last_occurredis nowwritten only on FAILED.
Found next to it and fixed here too: no event was published on the
CONFIRMED -> HEALEDtransition. The shipped
bringup_params.yamlsetshealing_enabled: true, so on the default stacka healed fault stayed on every SSE consumer's screen indefinitely. It now publishes
EVENT_CLEAREDwith
status: HEALED- deliberately the existing event name, since a new one would be silentlydropped by any UI not listening for it.
The SSE drop counter measured the wrong thing
The handler trimmed a shared 100-entry replay buffer FIFO and counted every eviction as a
client-visible drop, with no notion of clients at all. On the box the counter passed 20,000 while
SSE fault client connectedappeared zero times in the log - nothing had ever been lost, the ringwas simply rotating, and the "slow or disconnected clients" text was fiction.
Eviction is now cursor-aware. Entries every live client has already received go first and cost
nothing. Entries superseded by a newer event for the same fault code go next, so a lagging client
still converges on the current state of every fault; entries carrying
auto_cleared_codesareexempt, because those symptom codes appear nowhere else. Only what neither rule covers is counted
and logged as an actual loss, with the slow-client, coalesced and reaped counts in the message.
A client with no completed write for three keepalive intervals is reaped and excluded from the
accounting rather than inflating backpressure forever.
Because coalescing erases mid-deque, the stream loop now formats under the lock and writes outside
it; the previous code held a
dequeiterator acrosssink.write.Verification
Built and tested in the CI image (Jazzy, CycloneDDS). Gateway 3959 tests, fault_manager 527 tests,
zero failures.
clang-format-18run deliberately rather than left to be skipped: 438 files in thegateway, 36 in fault_manager, clean.
Each fix has a test that fails when it is reverted:
FaultStorageTest.PassedEventDoesNotAdvanceLastOccurredand the sqlite equivalentHealingFaultEventPublishingTest- "no event published for the CONFIRMED -> HEALED transition"BufferRotationWithNoClientsIsNotADrop,LaggingClientKeepsRareFaultWhenFlapFillsBuffer,StalledClientIsReapedInsteadOfCountedAsBackpressuretest_graph_provider_{sse,greenwave,stale}inros2_medkit_integration_testsfail in thiscontainer, and also fail on an unmodified
origin/mainbuild with the same signature(
rclcpp::graph_listener::NodeNotFoundErroraborting the gateway). Pre-existing, not from thischange.