Keep more than one rosbag per fault, and stop losing the evidence around it

[mfaferek93]

Closes #620.

Only the newest rosbag was ever kept per fault code. That was enforced by the schema, not by a retention policy, so no configuration could change it: a fault that kept coming back held on to its freeze frame and threw away every recording but the last.

The fix separates three things the old code treated as one:

• a link - some fault claiming some recording - keyed by (fault_code, file_path), one row
• a recording - one bag on disk - identified by file_path, named publicly by recording_id
• the bytes, which belong to file_path

Rows are per link, bytes are per path, and a bag is unlinked when its last row goes. The reverse relation already worked: a burst writes one row per attached fault, all pointing at one bag, so the sharing logic and the quota needed no changes. This adds the mirror image.

Retention

snapshots.rosbag.max_bags_per_fault, default 1, which is what the code did before: a new recording replaces the old one. Shipping at parity means a regression report is about the plumbing rather than the policy. Past the cap the oldest goes, matching the direction the storage quota already evicts in.

It is more a fairness knob than a depth knob. max_total_storage_mb is the real disk bound, and a big per-fault cap lets one flapping fault eat the budget and push out everyone else's black box. Docs say so.

Not consistent with snapshots.max_per_fault, which rejects new snapshots once full, and deliberately so. Rejecting a new recording would mean a technician standing next to a machine that is faulting right now downloads a bag from three days ago.

URLs

/bulk-data/rosbags/{id} takes a recording id now. Our own docs and the SSE payload tell clients to build that URL from a fault code, so an id that names no recording is retried as one and serves that fault's newest bag. Old addresses keep working, hence feat and not feat!.

Four bugs found on the way

None were reachable while a fault could only hold one recording:

• quota eviction deleted by fault code, so evicting one bag would have wiped every recording of every fault it touched, up to 33 in one burst
• get_rosbag_file had no ORDER BY and would have served an arbitrary recording
• the two stale-row self-heals deleted a fault's whole history when one bag went missing from disk
• delete_rosbag_file and delete_rosbag_files read only the first file_path, so deleting a fault removed every row but left all but one bag on disk, unreachable and still counted against a quota that sums rows

Migration

SQLite cannot drop a column-level UNIQUE and CREATE TABLE IF NOT EXISTS does nothing to an existing database, so this is the documented table rebuild. It runs on first open, is idempotent, and decides whether it is needed by looking at the schema (PRAGMA index_list, origin 'u') rather than a version counter - fresh, migrated and already-migrated databases all answer correctly with no bookkeeping.

Uniqueness is a CREATE UNIQUE INDEX on (fault_code, file_path) now. Keyed on the path because that is what identifies the thing on disk: if two bags ever produced the same basename, keying on recording_id would silently replace one, while keying on the path degrades the same collision to a mislabeled download.

Tests

6871 pass. Each suite below was also run with the fix reverted, to check it goes red for the right reason.

test_rosbag_storage_parity.cpp is new: 20 cases, each run against both storage backends from one body. The in-memory backend enforced one-per-fault independently, so storage_type: memory could have kept the old behaviour with nothing failing. It caught two real divergences - the delete-path bug above, and INSERT OR REPLACE renumbering a refreshed row so the backends ordered a tie group differently.

test_rosbag_history.test.py drives confirm, acknowledge, confirm against SQLite, plus cap eviction and both ways of addressing a bag. Set max_bags_per_fault to 1 and its first case fails, which is the reported bug.

test_rosbag_history_download.test.py does the same over HTTP through the gateway, off a real failure: lidar_sensor checks min_range >= max_range and reports itself, the test only moves the parameters. It asserts two descriptors with different ids, different downloaded bytes, both bulk_data_uris resolving, and the fault-code URL still serving the newest.

Migration coverage builds a legacy database by hand and checks that no origin='u' index survives, every row does, ids keep their order, recording_id is backfilled, and opening twice more changes nothing.

Three more places evidence went missing

Found while checking the above against a running box, and fixed here because
shipping the retention change without them leaves a fault whose evidence does
not line up.

A recording reached clients with no capture time. The fault manager has
always stamped one, but only the freeze-frame branch of the transport forwarded
it, so every bag arrived without one and the UIs rendered "N/A". Harmless while
a fault held one recording; with several the timestamp is the only field that
places an occurrence.

A capture was stored in part. A confirmation writes one snapshot row per
topic and the cap counted rows, rejecting the tail once full - so a capture
straddling the limit landed with some topics present and the rest silently
absent, which on the wire reads exactly like "that topic was not publishing".
Captures are now written as a unit and the cap drops whole ones, oldest first.
A capture larger than the cap is kept whole rather than torn.

Acknowledging a fault deleted the history it was told to keep.
auto_cleanup defaults on and deletes every recording of the fault, so raising
max_bags_per_fault to collect a trail bought nothing: the first
acknowledgement wiped it. With a history configured the cap governs retention;
at the default of one, nothing changes. clear_fault deleted the value
snapshots on the same edge, so the new snapshots.retain_on_clear keeps those
too - both survive an acknowledgement or neither does, rather than a fault being
left holding bags whose readings are gone.

Each has a test that goes red when its fix is reverted, on both storage backends
where the storage layer is involved.

Consumers

Four needed changes, each with its own PR: ros2_medkit_web_ui#95, platform#270 (fleet_ui), ros2_medkit_foxglove_extension#21, ros2_medkit_mcp#24. Both web UIs were checked in a browser against a gateway holding two real bags. The service type hashes change, so the gateway and fault manager deploy together.

[Copilot]

Pull request overview

This PR fixes the “one rosbag per fault_code” limitation by introducing a recording-centric identity (recording_id) and a retention policy (snapshots.rosbag.max_bags_per_fault, default 1 to preserve prior behavior). It updates the fault manager’s storage schema/migration and the gateway’s bulk-data surface so multiple recordings per fault are addressable and downloadable while keeping legacy fault-code URLs working via a compatibility fallback.

Changes:

• Fault manager: change rosbag storage grain to one row per (fault_code, file_path) link, persist/index recording_id, add per-fault retention (keep-newest), and migrate legacy SQLite schemas automatically/idempotently.
• Gateway: address rosbag bulk-data by recording id, fold list output into one descriptor per recording, and authorize downloads against the union of faults attached to that recording (with legacy fault-code URL fallback).
• Messages/docs/tests: extend GetRosbag / ListRosbags / Snapshot semantics, update documentation, and add unit + integration coverage for history/compatibility/migration.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/ros2_medkit_msgs/srv/ListRosbags.srv	Adds recording_ids[] parallel to fault_codes[] for per-link rows.
src/ros2_medkit_msgs/srv/GetRosbag.srv	Adds request recording_id (preferred) and response recording_id + fault_codes[].
src/ros2_medkit_msgs/msg/Snapshot.msg	Clarifies bulk_data_id now carries rosbag recording id (not fault code).
src/ros2_medkit_msgs/CHANGELOG.rst	Documents service/message surface changes and coordinated deployment requirement.
src/ros2_medkit_integration_tests/test/features/test_rosbag_history_download.test.py	New E2E HTTP test covering multi-recording history + legacy fault-code URL behavior.
src/ros2_medkit_integration_tests/test/features/test_rosbag_boundary_download.test.py	Updates descriptor lookup and download expectations to recording-id model.
src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py	Updates rollup/bulk-data URI assertions for recording ids + legacy URL parity.
src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py	Updates descriptor schema expectations (fault_codes[], recording_id, id equality).
src/ros2_medkit_integration_tests/CMakeLists.txt	Extends timeout for the new longer-running history download test.
src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp	Adds unit tests for descriptor folding + attached-fault-code download authorization.
src/ros2_medkit_gateway/src/ros2/transports/ros2_fault_service_transport.cpp	Populates new GetRosbag/ListRosbags fields and tightens parallel-array validation.
src/ros2_medkit_gateway/src/ros2/conversions/fault_msg_conversions.cpp	Renames rosbag snapshot JSON field to bulk_data_id (recording id).
src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp	Updates SSE comment to reflect legacy fault-code URL now being compatibility behavior.
src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp	Builds bulk-data URIs from bulk_data_id (recording id) and omits unaddressable entries.
src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp	Implements descriptor folding (per recording), recording-id downloads, and union authorization.
src/ros2_medkit_gateway/README.md	Updates SSE consumer guidance re: newest recording vs listing for older recordings.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp	Documents/declares recording id helpers and folding utilities.
src/ros2_medkit_gateway/CHANGELOG.rst	Documents the recording-id bulk-data API behavior and compatibility fallback.
src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp	Adds migration + retention + link/recording deletion unit coverage for SQLite backend.
src/ros2_medkit_fault_manager/test/test_rosbag_storage_parity.cpp	New cross-backend parity suite for retention/link/ordering behavior.
src/ros2_medkit_fault_manager/test/test_rosbag_history.test.py	New launch test driving confirm/clear/reconfirm history against SQLite backend services.
src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp	Updates quota sweep failing-storage hook to new delete-by-recording API.
src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp	Implements schema changes, migration, per-fault cap trimming, and recording-based deletion.
src/ros2_medkit_fault_manager/src/rosbag_capture.cpp	Stores recording_id and evicts over quota by recording (not by fault code).
src/ros2_medkit_fault_manager/src/fault_storage.cpp	Adds shared rosbag_recording_id() utility and rewrites in-memory backend to link-grain.
src/ros2_medkit_fault_manager/src/fault_manager_node.cpp	Adds recording-id validation and recording-first GetRosbag lookup with fallback-to-fault-code.
src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp	Exposes new rosbag APIs (cap, list, drop link, delete recording, lookup by recording).
src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp	Adds config field max_bags_per_fault.
src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp	Defines recording-id semantics and new storage APIs for link/recording operations.
src/ros2_medkit_fault_manager/config/snapshots.yaml	Documents new max_bags_per_fault and recording-id URL semantics.
src/ros2_medkit_fault_manager/CMakeLists.txt	Registers new gtest + launch tests and sets timeouts.
src/ros2_medkit_fault_manager/CHANGELOG.rst	Documents retention policy, schema grain change, migration approach, and fixed latent defects.
docs/tutorials/snapshots.rst	Updates bulk-data URIs/listing examples and explains per-recording listing behavior.
docs/config/fault-manager.rst	Documents rosbag.max_bags_per_fault and its fairness semantics + interaction with auto_cleanup.
docs/api/rest.rst	Updates REST API examples and clarifies “newest recording” compatibility URL semantics.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.