feat: zero-config freeze-frame - capture the faulting entity's own data by default#538
Merged
Merged
Conversation
…ta by default Fault manager: when no snapshot config matches a fault code, capture the reporting source node's own published topics (snapshots.entity_default, default on). Gateway: on confirm of a plugin-entity fault, snapshot the entity's current DataProvider values and merge them into the fault detail (entity_freeze_frame.enabled, default on). Explicit config always wins. Closes #537
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements “zero-config freeze-frame” capture so that every confirmed fault includes at-fault-time context by default, even when no snapshot configuration matches. It adds a fault-manager fallback for ROS-backed entities (capture the reporting node’s own topics) and a gateway-side capture path for plugin-backed (PLC-style) entities (snapshot current values via the owning plugin’s DataProvider), while preserving precedence where explicit snapshot configuration always wins.
Changes:
- Fault manager: add
snapshots.entity_default(defaulttrue) and implement entity-default topic resolution + capture behavior when no explicit snapshot config matches. - Gateway: add
entity_freeze_frame.enabled(defaulttrue) and implementEntityFreezeFrameCapture+ response merging so plugin-backed entities contribute freeze-frames when the fault manager did not. - Add unit + integration tests plus documentation updates for the new defaults and precedence rules.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/test/features/test_entity_freeze_frame.test.py | End-to-end test proving zero-config freeze-frame for plugin and ROS entities |
| src/ros2_medkit_gateway/test/test_entity_freeze_frame_capture.cpp | Unit tests for gateway-side plugin entity capture + merge precedence |
| src/ros2_medkit_gateway/test/demo_nodes/test_data_provider_plugin.cpp | Demo plugin used by integration test to simulate PLC-style DataProvider-backed entity |
| src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp | Merge plugin-derived freeze-frames into fault detail when fault manager didn’t capture any |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Add entity_freeze_frame.enabled parameter and wire capture lifecycle |
| src/ros2_medkit_gateway/src/entity_freeze_frame_capture.cpp | New capture component: subscribe to fault events and snapshot plugin entity values on confirm |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/fault_handlers.hpp | Expose merge helper for testability |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp | Expose get_entity_freeze_frame_capture() to handlers |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/entity_freeze_frame_capture.hpp | Public API for gateway-side entity freeze-frame capture |
| src/ros2_medkit_gateway/CMakeLists.txt | Build new capture source, add GTest target, build/install demo plugin |
| src/ros2_medkit_fault_manager/test/test_snapshot_capture.cpp | Unit tests for entity-default fallback behavior and precedence |
| src/ros2_medkit_fault_manager/src/snapshot_capture.cpp | Implement entity-default resolution + on-demand-only capture behavior |
| src/ros2_medkit_fault_manager/src/fault_manager_node.cpp | Add snapshots.entity_default param and ignore empty default_topics entries |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp | Document/declare new entity_default behavior |
| src/ros2_medkit_fault_manager/config/snapshots.yaml | Document precedence including entity-default fallback |
| docs/tutorials/snapshots.rst | Document zero-config entity freeze-frames and opt-out flags |
| docs/config/server.rst | Document entity_freeze_frame.enabled gateway parameter |
| docs/config/fault-manager.rst | Document snapshots.entity_default fault-manager parameter |
The zero-config freeze-frame capture created a naked create_subscription on the main node, tripping the issue #375 regression gate. Subscribe via the shared Ros2SubscriptionExecutor instead (wired in main once the executor exists), and update the unit test to drive the slot path.
Plugins without a DataProvider (the commercial PLC bridges) serve live values only through their registered routes, so entity freeze-frames came out empty. Dispatch the owning plugin's x-plc-data route in-process at fault confirm, gated on connected + non-empty items.
…rovider Plugins that report via ReportFault only (s7comm, modbus) got a 404 from the per-entity fault detail, hiding the fault manager's freeze-frames. Fall through to the shared scoped listing instead.
A bare plugin entity id parsed as (name, ns="/") could match an unrelated root-namespace node of the same name and freeze-frame its topics. Also document and pin the gateway frame retention semantics: kept across clear, overwritten on every re-confirm.
bburda
reviewed
Jul 17, 2026
Collaborator
There was a problem hiding this comment.
Additional findings outside the diff:
- The in-process x-plc-data dispatch now invokes a plugin's route handler from the subscription worker, concurrently with a real HTTP GET of the same route. Before this, only httplib threads called it. A handler with unguarded mutable state now races; worth a thread-safety line in the plugin route contract. The plugins_mutex_ re-entrancy itself is correctly handled by copy-then-release.
- fallback_logged_ is only inserted, never evicted, so it grows unbounded with distinct fault codes (churny or synthetic codes on a flaky bridge); and the DataProvider branch WARNs on every re-confirm with no once-guard (the route path dedups). Bound the set or add a once-guard.
- Two safety properties are implicit; pin them with a comment or test: (a) ~EntityFreezeFrameCapture is not a synchronous barrier (subscription_slot_.reset() waits a 30s deadline), so use-after-free safety relies on main.cpp resetting the executor (joining the worker) before the node; a reorder reintroduces a use-after-free. (b) frames_ is keyed by fault_code only; cross-entity isolation holds only because reporting_sources is monotonic and get_fault gates on source-scope; a future per-source clear would let a stale frame leak.
- Gateway entity frames are not evicted on EVENT_CLEARED (only CONFIRMED is handled), while the fault_manager deletes its own snapshots on clear. A cleared-but-retained fault could surface a gateway freeze-frame with no fault_manager counterpart. Confirm whether cleared faults stay queryable.
- Eviction, the gateway opt-out and gating, and the route-dispatch re-entrancy guard are untested (the C++ unit uses a fake fetcher, so the deadlock the design guards against is unproven). The design docs do not mention the new entity-default fallback or the EntityFreezeFrameCapture component; only the config docs and tutorial do.
- Doc: "explicit snapshot config always wins" is not exact; a rosbag-only capture does not suppress the entity freeze-frame (only a freeze_frame does).
Run plugin capture on a dedicated worker so a slow list_data/x-plc-data
read cannot stall the shared subscription executor. Gate both capture
paths on live values (no {} / all-null rows), make the item parser total
for non-string ids, and bound the once-per-code warn set.
A fault code present in fault_specific or matched by a pattern no longer falls through to entity-default capture when its topic list is empty - that is an explicit "capture nothing" for the code.
bburda
approved these changes
Jul 17, 2026
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.
Today a fault carries freeze-frame context only when snapshot config matches (
fault_specific/patterns/default_topics), and PLC-backed entities have no working path at all. This makes at-fault-time context the default: every confirmed fault carries the faulting entity's own current data, zero config.Two complementary pieces, both default-ON with an opt-out; explicit snapshot config always wins:
snapshots.entity_default): when nothing else matches, capture the topics published by the fault's reporting node (excludes/rosout,/parameter_events, capped at 16). Empty run writes no row, preserving "no row = nothing captured".entity_freeze_frame.enabled): PLC values aren't ROS topics, so the gateway snapshots the entity's current values from the owning plugin (no plugin API change) and merges them intoenvironment_data.snapshotsas standardfreeze_frameentries - skipped when a freeze-frame was already captured.Precedence:
fault_specific>patterns>default_topics> entity-default.Tests: unit coverage for both paths (precedence, opt-out, plugin vs ROS source) plus an end-to-end test with no snapshot config proving a plugin fault carries entity values and a ROS fault carries its node's topics.
Closes #537