From 6abd228cee7b3517b6166162accb9a5dfd234722 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 8 Sep 2026 22:01:41 +0200 Subject: [PATCH 1/6] fix(gateway): size a rosbag descriptor by the bytes the download serves A rosbag2 recording is a directory with one storage file and metadata.yaml. The download route resolves the storage file and streams only that file. The listing reported the figure the fault manager stores, which is the size of the whole directory. The two numbers answer different questions. The stored figure is the footprint of the recording against its disk quota. The listed figure is what a client is about to fetch. So every listing overstated the download, on a short recording by about a tenth of the transfer. A client that sized a buffer or a progress bar from the listing never reached the end. The listing now measures the file that the download resolves, through the same resolver. The promised length is the length that arrives. resolve_rosbag_file_path becomes a public static member so the listing can call it. When this process cannot see the bag, the descriptor keeps the stored figure. It is the only number available, and a zero would describe the recording as empty. The stored figure itself is unchanged, so quota accounting still counts the bytes the recording occupies. rest.rst and the DTO comment state that size is the byte count the download serves. Also drop the removed snapshot endpoints from the gateway README and from the quick start of the snapshots tutorial. GET /faults/{code}/snapshots and .../snapshots/bag answer 404. A fault returns its snapshots inline, and recordings are downloaded through the bulk-data endpoints. The migration table in the tutorial stays, because it points a reader at the replacements. --- docs/api/rest.rst | 6 + docs/tutorials/snapshots.rst | 6 +- src/ros2_medkit_gateway/README.md | 131 +++--------------- .../core/http/handlers/bulkdata_handlers.hpp | 56 ++++++-- .../ros2_medkit_gateway/dto/bulkdata.hpp | 4 +- .../src/http/handlers/bulkdata_handlers.cpp | 34 ++++- .../test/test_bulkdata_handlers.cpp | 101 ++++++++++++++ 7 files changed, 210 insertions(+), 128 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 0321f22fc..2ac5f9d38 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1772,6 +1772,12 @@ recording therefore reports its size once. One fault code can appear on several descriptors, one per occurrence it kept, told apart by ``creation_date``, which is the time that recording was made. +``size`` is the number of bytes the download route below puts on the wire for +that descriptor, so a client can size a buffer or a progress bar from the +listing. For a rosbag that is the bag's single storage file (``.mcap`` or +``.db3``), which is the only file the download serves. The bag directory also +holds ``metadata.yaml``, and those bytes are not part of the transfer. + Download Bulk Data ~~~~~~~~~~~~~~~~~~ diff --git a/docs/tutorials/snapshots.rst b/docs/tutorials/snapshots.rst index f17d55d7e..79e0cf797 100644 --- a/docs/tutorials/snapshots.rst +++ b/docs/tutorials/snapshots.rst @@ -52,11 +52,13 @@ Quick Start ros2 launch ros2_medkit_gateway gateway.launch.py -3. **When a fault is confirmed, query its snapshots:** +3. **When a fault is confirmed, read its snapshots from the fault itself:** + + They are returned inline, under ``environment_data.snapshots``. .. code-block:: bash - curl http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots + curl http://localhost:8080/api/v1/apps/motor_controller/faults/MOTOR_OVERHEAT Configuration Options --------------------- diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index 0cff2b0d3..decd82459 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -980,13 +980,14 @@ Faults represent errors or warnings reported by system components. The gateway p - `GET /api/v1/faults` - List all faults across the system (convenience API for dashboards) - `GET /api/v1/faults/stream` - Real-time fault event stream via Server-Sent Events (SSE) -- `GET /api/v1/faults/{fault_code}/snapshots` - Get topic snapshots captured when fault was confirmed -- `GET /api/v1/faults/{fault_code}/snapshots/bag` - Download rosbag file for fault (if rosbag capture enabled) - `GET /api/v1/components/{component_id}/faults` - List faults for a specific component - `GET /api/v1/components/{component_id}/faults/{fault_code}` - Get a specific fault -- `GET /api/v1/components/{component_id}/faults/{fault_code}/snapshots` - Get snapshots for a component's fault - `DELETE /api/v1/components/{component_id}/faults/{fault_code}` - Clear a fault +Snapshots are not a separate endpoint. A fault response carries them inline in +`environment_data.snapshots[]`, and a rosbag recording is downloaded through the +bulk-data endpoints (`GET /api/v1/{entity-path}/bulk-data/rosbags/{id}`). + #### GET /api/v1/faults List all faults across the system. This is a convenience API for dashboards and monitoring tools that need a complete system health view without iterating over individual components. @@ -1103,79 +1104,12 @@ curl http://localhost:8080/api/v1/components/nav2_controller/faults } ``` -#### GET /api/v1/faults/{fault_code}/snapshots - -Get topic snapshots captured when a fault transitioned to CONFIRMED status. Snapshots provide system state at the moment of fault confirmation for debugging purposes. - -**Query Parameters:** -- `topic` - (optional) Filter by specific topic name - -**Example:** -```bash -curl http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots -curl http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots?topic=/joint_states -``` +#### Snapshots -**Response (200 OK):** -```json -{ - "fault_code": "MOTOR_OVERHEAT", - "captured_at": 1735830000.123, - "topics": { - "/joint_states": { - "message_type": "sensor_msgs/msg/JointState", - "data": {"name": ["joint1"], "position": [1.57]} - }, - "/cmd_vel": { - "message_type": "geometry_msgs/msg/Twist", - "data": {"linear": {"x": 0.5}, "angular": {"z": 0.1}} - } - } -} -``` - -**Response (200 OK - No snapshots):** -```json -{ - "fault_code": "MOTOR_OVERHEAT", - "topics": {} -} -``` - -**Response (404 Not Found):** -```json -{ - "error": "Fault not found", - "fault_code": "NONEXISTENT_FAULT" -} -``` - -#### GET /api/v1/components/{component_id}/faults/{fault_code}/snapshots - -Get topic snapshots for a specific component's fault. Same as the system-wide endpoint but scoped to a component. - -**Query Parameters:** -- `topic` - (optional) Filter by specific topic name - -**Example:** -```bash -curl http://localhost:8080/api/v1/components/motor_controller/faults/MOTOR_OVERHEAT/snapshots -``` - -**Response (200 OK):** -```json -{ - "component_id": "motor_controller", - "fault_code": "MOTOR_OVERHEAT", - "captured_at": 1735830000.123, - "topics": { - "/motor/temperature": { - "message_type": "sensor_msgs/msg/Temperature", - "data": {"temperature": 85.5, "variance": 0.1} - } - } -} -``` +Snapshots captured when a fault transitioned to CONFIRMED are returned inline +with the fault itself, in `environment_data.snapshots[]` of +`GET /api/v1/{entity-path}/faults/{fault_code}`. There is no separate snapshot +endpoint. **Snapshot Configuration:** @@ -1214,45 +1148,18 @@ default_topics: - /diagnostics ``` -#### GET /api/v1/faults/{fault_code}/snapshots/bag - -Download the rosbag file associated with a fault. This endpoint is only available when rosbag capture is enabled in FaultManager. - -Rosbag capture provides "black box" style recording - a ring buffer continuously records configured topics, and when a fault is confirmed, the buffer is flushed to a bag file. This allows capturing system state both **before and after** fault confirmation. - -**Example:** -```bash -# Download rosbag archive -curl -O -J http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots/bag - -# Or save with custom filename -curl http://localhost:8080/api/v1/faults/MOTOR_OVERHEAT/snapshots/bag -o motor_fault.tar.gz -``` - -**Response (200 OK):** -- For directory-based bags (default rosbag2 format): compressed tar.gz archive containing the full bag directory with metadata.yaml and all storage segments -- Content-Type: `application/gzip` -- Content-Disposition: `attachment; filename="fault_MOTOR_OVERHEAT_20260124_153045.tar.gz"` +#### Rosbag Recordings -The archive can be extracted and played directly with `ros2 bag play`. +Rosbag capture provides "black box" style recording - a ring buffer continuously +records configured topics, and when a fault is confirmed the buffer is flushed to +a bag file. This captures system state both **before and after** fault +confirmation. -**Response (404 Not Found - Fault or rosbag not found):** -```json -{ - "error": "Rosbag not found", - "fault_code": "MOTOR_OVERHEAT", - "details": "No rosbag file associated with this fault" -} -``` - -**Response (404 Not Found - Rosbag file deleted):** -```json -{ - "error": "Rosbag file not found", - "fault_code": "MOTOR_OVERHEAT", - "details": "File was deleted or moved" -} -``` +A recording is listed and downloaded through the bulk-data endpoints: +`GET /api/v1/{entity-path}/bulk-data/rosbags` for the descriptors and +`GET /api/v1/{entity-path}/bulk-data/rosbags/{recording_id}` for the bytes. The +download serves the bag's single storage file (`.mcap` or `.db3`), and the +descriptor `size` is that file's length. **Rosbag Configuration:** diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp index 5786d7222..f61b03f1d 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp @@ -14,6 +14,8 @@ #pragma once +#include +#include #include #include #include @@ -102,6 +104,23 @@ class BulkDataHandlers { */ static std::vector download_media_types(); + /** + * @brief Resolve rosbag file path from storage path. + * + * Rosbag2 creates a directory containing the actual db3/mcap file. + * This function resolves the directory to the actual file path. + * + * The single place that decides which bytes a recording IS. `download()` + * streams the file this returns and reports its length. The listing sizes + * its descriptor from the same file through `detail::rosbag_served_bytes`. + * Both must move together, which is why this is reachable from outside the + * class rather than a private helper of the download path. + * + * @param path Path to rosbag (can be file or directory) + * @return Resolved file path, or empty string if not found + */ + static std::string resolve_rosbag_file_path(const std::string & path); + private: HandlerContext & ctx_; @@ -114,17 +133,6 @@ class BulkDataHandlers { * to keep the handler's public surface unchanged. */ std::vector get_source_filters(const EntityInfo & entity) const; - - /** - * @brief Resolve rosbag file path from storage path. - * - * Rosbag2 creates a directory containing the actual db3/mcap file. - * This function resolves the directory to the actual file path. - * - * @param path Path to rosbag (can be file or directory) - * @return Resolved file path, or empty string if not found - */ - static std::string resolve_rosbag_file_path(const std::string & path); }; namespace detail { @@ -205,6 +213,27 @@ std::vector rosbag_attached_fault_codes(const nlohmann::json & rosb */ bool rosbag_resolved_by_fault_code(const nlohmann::json & rosbag_data, const std::string & requested_id); +/** + * @brief Bytes a rosbag download puts on the wire for one recording. + * + * ``BulkDataHandlers::resolve_rosbag_file_path`` picks the single storage file + * inside the bag directory and the download streams that file alone, so the + * length a client is told to expect is that file's length and nothing else. + * + * The fault manager's stored ``size_bytes`` answers a different question. It + * walks the whole bag directory, because it is the figure the recording's disk + * quota is spent against, and the directory also holds ``metadata.yaml``. + * Reporting that figure as the descriptor size overstated every download by the + * metadata file - on a short recording, by around a tenth of the transfer - and + * a client sizing a buffer or a progress bar from the listing never reached the + * end. The listing therefore states what the download serves, measured on the + * file the download resolves, and leaves the quota figure to the quota. + * + * @param bag_path Bag path as stored by the fault manager (directory or file) + * @return The resolved file's size, or nullopt when this process cannot see it + */ +std::optional rosbag_served_bytes(const std::string & bag_path); + /** * @brief Fold rosbag link rows into one descriptor per recording. * @@ -220,6 +249,11 @@ bool rosbag_resolved_by_fault_code(const nlohmann::json & rosbag_data, const std * Order follows first appearance, which is the order the fault manager listed * the rows in. * + * The descriptor size is measured on the file the download resolves (see + * ``rosbag_served_bytes``), not taken from the row. A row whose bag this + * process cannot see keeps the row's own figure: it is the only number left, + * and a recording listed with a zero size reads as an empty one. + * * @param rows Rosbag rows as returned by the fault manager * @param faults_by_code Faults keyed by code, for timestamp enrichment * @return One descriptor per distinct recording diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/bulkdata.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/bulkdata.hpp index 69dd40d43..9c8ca8acb 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/bulkdata.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/bulkdata.hpp @@ -55,7 +55,9 @@ inline constexpr std::string_view dto_name = "BulkDataCate // id - unique file identifier (required) // name - human-readable filename / label (required) // mimetype - MIME type of the file (required) -// size - byte count (required) +// size - byte count the download route serves for this item +// (required). For a rosbag that is the bag's single +// storage file, not the bag directory's total // creation_date - ISO 8601 timestamp string (required) // description - optional human-readable description // x-medkit - optional open vendor extension object; for rosbags: diff --git a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp index df98c93f8..26bced8e5 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -121,6 +122,25 @@ std::string rosbag_recording_id(const std::string & file_path) { return p.filename().string(); } +std::optional rosbag_served_bytes(const std::string & bag_path) { + if (bag_path.empty()) { + return std::nullopt; + } + // The same two steps `download()` performs, in the same order and through the + // same resolver, so the size a client is promised cannot drift from the size + // it is sent. Changing which file a recording resolves to changes both. + const std::string resolved = BulkDataHandlers::resolve_rosbag_file_path(bag_path); + if (resolved.empty()) { + return std::nullopt; + } + std::error_code ec; + const auto size = std::filesystem::file_size(resolved, ec); + if (ec) { + return std::nullopt; + } + return static_cast(size); +} + std::vector rosbag_attached_fault_codes(const nlohmann::json & rosbag_data, const std::string & requested_id) { if (rosbag_data.contains("fault_codes") && rosbag_data["fault_codes"].is_array()) { @@ -206,7 +226,14 @@ fold_rosbag_rows_into_descriptors(const std::vector & rows, // Default to sqlite3 (the historical FaultManager default) when a bag predates // the persisted format field; the per-bag metadata normally carries the real one. entry.format = row.value("format", "sqlite3"); - entry.size_bytes = row.value("size_bytes", uint64_t{0}); + // What the download route will actually send, measured on the file it + // resolves. The stored figure is the bag directory's total, which is the + // recording's footprint against the disk quota and not its transfer size - + // it counts metadata.yaml, which the download does not serve. Keep the + // stored figure only when the bag is not visible from this process: it is + // then the only number available, and listing a zero would describe the + // recording as empty rather than as unmeasured. + entry.size_bytes = rosbag_served_bytes(row.value("file_path", "")).value_or(row.value("size_bytes", uint64_t{0})); entry.duration_sec = row.value("duration_sec", 0.0); entry.created_at_ns = created_at_ns; entry.fault_codes.push_back(fault_code); @@ -491,7 +518,10 @@ http::Result BulkDataHandlers::download(const http::TypedR // URL is not the segment the client sent. filename = rosbag_result.data.value("recording_id", bulk_data_id) + "." + format; - // Rosbag2 emits a directory layout - resolve the inner db3/mcap file. + // Rosbag2 emits a directory layout - resolve the inner db3/mcap file. Only + // that file is served, and metadata.yaml stays on the gateway host. The listing + // sizes its descriptor through detail::rosbag_served_bytes, which resolves + // the same way, so the Content-Length below is the number it advertised. actual_path = resolve_rosbag_file_path(file_path); } else { // === Non-rosbag categories: served via BulkDataStore === diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index c98d65ea1..d3ba1048b 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -15,9 +15,13 @@ #include #include +#include +#include +#include #include #include #include +#include #include #include @@ -235,6 +239,8 @@ TEST_F(BulkDataHandlersTest, ARowWithNeitherIdNorPathIsDroppedRatherThanAdvertis } TEST_F(BulkDataHandlersTest, DistinctRecordingsEachReportTheirOwnSize) { + // The paths in these rows do not exist on this host, so each descriptor keeps + // the row's own figure - the fallback the sizing test below covers explicitly. const std::vector rows{rosbag_row("A", "fault_A_1", 2048), rosbag_row("B", "fault_B_1", 4096)}; const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); @@ -247,6 +253,101 @@ TEST_F(BulkDataHandlersTest, NoRowsYieldsNoDescriptors) { EXPECT_TRUE(handlers::detail::fold_rosbag_rows_into_descriptors({}, {}).empty()); } +// === Descriptor size vs served bytes === +// A rosbag2 bag is a directory: one storage file plus metadata.yaml. The +// download resolves the storage file and streams that alone, so the descriptor +// has to be sized on the same file. The fault manager's stored figure is the +// directory total, which is the recording's disk footprint and larger than the +// transfer. Reporting it made every listing overstate the download. + +class RosbagBagDirectoryTest : public ::testing::Test { + protected: + void SetUp() override { + bag_dir_ = std::filesystem::temp_directory_path() / + ("bulkdata_bag_test_" + std::to_string(getpid()) + "_" + std::to_string(counter_++)); + std::filesystem::create_directories(bag_dir_); + write_file(bag_dir_ / "recording_0.db3", std::string(4096, 'x')); + write_file(bag_dir_ / "metadata.yaml", std::string(311, 'y')); + } + + void TearDown() override { + std::error_code ec; + std::filesystem::remove_all(bag_dir_, ec); + } + + static void write_file(const std::filesystem::path & path, const std::string & content) { + std::ofstream out(path, std::ios::binary); + out << content; + } + + // What the fault manager stores: every regular file under the bag directory. + uint64_t directory_total() const { + uint64_t total = 0; + for (const auto & entry : std::filesystem::recursive_directory_iterator(bag_dir_)) { + if (entry.is_regular_file()) { + total += static_cast(entry.file_size()); + } + } + return total; + } + + std::filesystem::path bag_dir_; + static int counter_; +}; + +int RosbagBagDirectoryTest::counter_ = 0; + +TEST_F(RosbagBagDirectoryTest, DescriptorSizeIsTheBytesTheDownloadServesNotTheBagDirectoryTotal) { + // The two operations download() performs to fill Content-Length: resolve the + // bag directory to its storage file, then take that file's size. + const std::string served_path = BulkDataHandlers::resolve_rosbag_file_path(bag_dir_.string()); + ASSERT_EQ(served_path, (bag_dir_ / "recording_0.db3").string()); + const auto served_bytes = static_cast(std::filesystem::file_size(served_path)); + + // Not vacuous: the directory holds metadata.yaml as well, so the stored figure + // and the served figure are genuinely different numbers. + ASSERT_GT(directory_total(), served_bytes); + + // The row carries the directory total, which is what the fault manager stores. + const json row{{"fault_code", "MOTOR_OVERHEAT"}, + {"recording_id", bag_dir_.filename().string()}, + {"file_path", bag_dir_.string()}, + {"format", "sqlite3"}, + {"duration_sec", 6.0}, + {"size_bytes", directory_total()}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, served_bytes) << "the listing must promise the bytes the download sends"; + EXPECT_NE(descriptors[0].size, directory_total()) << "metadata.yaml is not served, so it must not be counted"; +} + +TEST_F(RosbagBagDirectoryTest, ServedBytesIsUnknownRatherThanZeroWhenTheBagIsNotVisible) { + // Positive control for the absence below: the same helper does answer for a + // bag it can see, so a nullopt is the missing bag and not a broken helper. + ASSERT_TRUE(handlers::detail::rosbag_served_bytes(bag_dir_.string()).has_value()); + + EXPECT_FALSE(handlers::detail::rosbag_served_bytes("").has_value()); + EXPECT_FALSE(handlers::detail::rosbag_served_bytes((bag_dir_ / "no_such_bag").string()).has_value()); + + // An empty bag directory resolves to no storage file at all. + const auto empty_bag = bag_dir_ / "empty_bag"; + std::filesystem::create_directories(empty_bag); + EXPECT_FALSE(handlers::detail::rosbag_served_bytes(empty_bag.string()).has_value()); +} + +TEST_F(RosbagBagDirectoryTest, AnUnreachableBagKeepsTheStoredFigureRatherThanReportingZero) { + const json row{{"fault_code", "MOTOR_OVERHEAT"}, + {"recording_id", "fault_MOTOR_OVERHEAT_1738664999000"}, + {"file_path", (bag_dir_ / "gone").string()}, + {"format", "sqlite3"}, + {"size_bytes", 35943}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, 35943u); +} + // === Shared timestamp utility tests === // @verifies REQ_INTEROP_071 From 213eea77dfc5008811c3ef694812369ff70d7716 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 12:22:52 +0200 Subject: [PATCH 2/6] fix(fault-manager): report the bytes a recording's download serves A recording is stored as a directory and served as a single file, so it has two sizes. The row carried one number for both jobs: the directory total. That total is what the recording costs against max_total_storage_mb. Every API answer that quoted it overstated the download by metadata.yaml, on a short recording by about a tenth of the transfer. The environment_data.snapshots[] entry next to a download link was the worst case, because a client sizes its transfer from that number. rosbag_served_bytes() now answers the reporting question on its own. It measures the storage file that the bag's metadata.yaml names. It reads the metadata through rosbag2_storage::MetadataIo, the library that wrote it. GetFault, GetSnapshots, GetRosbag and ListRosbags report their size through it. RosbagFileInfo::size_bytes and the quota are unchanged. They still count the whole directory, which is what eviction frees. When no single served file can be named, the function returns the stored total. This covers a missing metadata.yaml, one that cannot be parsed, a named file that is gone, and a recording split across several storage files past the maximum bag size. None of these is an error. The stored total is a real measurement of the recording. A zero would describe it as empty. rest.rst now states the rule once. The descriptor size, the nested size_bytes and the Content-Length of the download are the same number, and that number is the size of the storage file. A split recording is the exception and reports its total. --- docs/api/rest.rst | 13 ++ .../rosbag_capture.hpp | 32 ++++ .../src/fault_manager_node.cpp | 10 +- .../src/rosbag_capture.cpp | 48 ++++++ .../test/test_fault_manager.cpp | 72 +++++++++ .../test/test_rosbag_capture.cpp | 149 ++++++++++++++++++ 6 files changed, 320 insertions(+), 4 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 2ac5f9d38..762416032 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1778,6 +1778,19 @@ listing. For a rosbag that is the bag's single storage file (``.mcap`` or ``.db3``), which is the only file the download serves. The bag directory also holds ``metadata.yaml``, and those bytes are not part of the transfer. +.. _rest-recording-size-rule: + +**One recording, one size.** The descriptor ``size`` here, the +``environment_data.snapshots[].size_bytes`` a fault reports for the same +recording, and the ``Content-Length`` of its download are the same number, and +that number is the storage file. A recording also has a footprint on the +gateway host, which is larger because the directory holds ``metadata.yaml`` as +well. That figure is what the recording spends against its storage quota and is +not reported by the API. The one case where the two coincide is a recording +split across several storage files, past the configured maximum bag size: the +download can hand over only one of them, no single file describes the transfer, +and the API reports the recording's total instead. + Download Bulk Data ~~~~~~~~~~~~~~~~~~ diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp index 10e003606..3e85ea31c 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp @@ -487,4 +487,36 @@ class RosbagCapture { bool dynamic_discovery_{false}; }; +/// Bytes a client receives when it downloads the recording at @p bag_path. +/// +/// A recording occupies a directory and is served as a single file. Those are two +/// different quantities and the fault manager needs both. ``RosbagFileInfo::size_bytes`` +/// is the directory total, because that is what the recording costs against +/// ``max_total_storage_mb`` and what eviction frees. This is the other one: the storage +/// file the download hands over, which is what a caller sizing a buffer or a progress +/// bar needs. Reporting the total in its place overstated every download by +/// ``metadata.yaml``, and on a short recording that is around a tenth of the transfer. +/// +/// The file is the one ``metadata.yaml`` names in ``relative_file_paths``, read through +/// the same library that wrote it, so this answers with the bag's own record of its +/// contents rather than by guessing from a file extension. +/// +/// Falls back to @p stored_total_bytes, never to zero, when no single served file can +/// be named: +/// - no ``metadata.yaml``, or one that cannot be read or parsed. +/// - ``relative_file_paths`` naming other than exactly one file. Past +/// ``max_bag_size_mb`` rosbag2 splits a recording across several storage files, and +/// then no single number describes the download at all. +/// - a named file that cannot be stat'd. +/// +/// None of those is an error worth logging. A pre-metadata bag and a split bag are +/// both normal, this runs once per reported row on every request, and the fallback is +/// a real measurement of the recording rather than a failure sentinel. A zero would +/// not be: it would describe the recording as empty. +/// +/// @param bag_path Bag directory as stored in ``RosbagFileInfo::file_path`` +/// @param stored_total_bytes The stored directory total, used as the fallback +/// @return Size of the served storage file, or @p stored_total_bytes +size_t rosbag_served_bytes(const std::string & bag_path, size_t stored_total_bytes); + } // namespace ros2_medkit_fault_manager diff --git a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp index 146f0e3b4..262ed9cf5 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -1158,7 +1158,9 @@ void FaultManagerNode::handle_get_fault(const std::shared_ptrduration_sec; - rosbag_json["size_bytes"] = rosbag_info->size_bytes; + rosbag_json["size_bytes"] = rosbag_served_bytes(rosbag_info->file_path, rosbag_info->size_bytes); rosbag_json["format"] = rosbag_info->format; rosbag_json["download_url"] = "/api/v1/faults/" + request->fault_code + "/snapshots/bag"; result["rosbag"] = rosbag_json; @@ -1598,7 +1600,7 @@ void FaultManagerNode::handle_get_rosbag(const std::shared_ptrfault_codes = attached_codes; response->format = rosbag_info->format; response->duration_sec = rosbag_info->duration_sec; - response->size_bytes = rosbag_info->size_bytes; + response->size_bytes = rosbag_served_bytes(rosbag_info->file_path, rosbag_info->size_bytes); RCLCPP_DEBUG(get_logger(), "GetRosbag returned file '%s' for %s", rosbag_info->file_path.c_str(), subject.c_str()); } @@ -1635,7 +1637,7 @@ void FaultManagerNode::handle_list_rosbags( response->file_paths.push_back(info.file_path); response->formats.push_back(info.format); response->durations_sec.push_back(info.duration_sec); - response->sizes_bytes.push_back(info.size_bytes); + response->sizes_bytes.push_back(rosbag_served_bytes(info.file_path, info.size_bytes)); response->created_at_ns.push_back(info.created_at_ns); } diff --git a/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp b/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp index 01d4b0c9e..40afd6238 100644 --- a/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp @@ -30,6 +30,8 @@ #include #include #include +#include +#include #include #include #include @@ -1325,6 +1327,12 @@ std::string RosbagCapture::generate_bag_path(const std::string & fault_code) con return base_path + "/" + bag_directory_name(fault_code, timestamp); } +// The recording's footprint, and the figure stored on its rows. It is what the +// recording costs against max_total_storage_mb and what evicting it frees, so it +// counts everything in the directory including metadata.yaml and every file of a +// split. rosbag_served_bytes() below is the other measurement of the same +// recording, the one the API reports, and the two are deliberately different +// numbers - see its comment for which question each answers. size_t RosbagCapture::calculate_bag_size(const std::string & bag_path) const { size_t total_size = 0; @@ -1345,6 +1353,46 @@ size_t RosbagCapture::calculate_bag_size(const std::string & bag_path) const { return total_size; } +// The bytes a download of this recording actually transfers, which is the one +// storage file the bulk-data route hands over. calculate_bag_size() above answers +// the storage question (what the recording costs on disk) and this one answers the +// client's question (what is about to arrive). Reporting the footprint in place of +// the transfer is what made every listing overstate its own download by +// metadata.yaml. Keeping them separate is what lets the quota stay honest while the +// API does. +// +// The served file is read out of the bag's own metadata.yaml rather than guessed +// from a file extension, so a bag that names something unexpected is still described +// by its own record. See the header for every fallback and why none of them logs. +size_t rosbag_served_bytes(const std::string & bag_path, size_t stored_total_bytes) { + try { + rosbag2_storage::MetadataIo metadata_io; + if (!metadata_io.metadata_file_exists(bag_path)) { + return stored_total_bytes; + } + + const rosbag2_storage::BagMetadata metadata = metadata_io.read_metadata(bag_path); + // Exactly one, or there is no single served file to measure. Zero means a bag + // that recorded nothing addressable. More than one means a split, where the + // download hands over one segment and the rest are unreachable through it - a + // defect of the download route, not something a size can paper over. + if (metadata.relative_file_paths.size() != 1) { + return stored_total_bytes; + } + + const std::filesystem::path storage_file = std::filesystem::path(bag_path) / metadata.relative_file_paths.front(); + std::error_code ec; + const auto served = std::filesystem::file_size(storage_file, ec); + if (ec) { + return stored_total_bytes; + } + return static_cast(served); + } catch (const std::exception &) { + // read_metadata throws on a metadata.yaml that cannot be read or parsed. + return stored_total_bytes; + } +} + std::vector RosbagCapture::evict_bags_over_quota(FaultStorage * storage, size_t max_bytes) { size_t current_bytes = storage->get_total_rosbag_storage_bytes(); if (current_bytes <= max_bytes) { diff --git a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp index 92b0abaaf..989504c56 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -26,12 +27,15 @@ #include #include +#include +#include #include #include "rclcpp/rclcpp.hpp" #include "ros2_medkit_fault_manager/fault_audit_log.hpp" #include "ros2_medkit_fault_manager/fault_manager_node.hpp" #include "ros2_medkit_fault_manager/fault_storage.hpp" +#include "ros2_medkit_fault_manager/rosbag_capture.hpp" #include "ros2_medkit_fault_manager/sqlite_fault_storage.hpp" #include "ros2_medkit_msgs/msg/fault.hpp" #include "ros2_medkit_msgs/msg/fault_event.hpp" @@ -1551,6 +1555,74 @@ TEST_F(FreezeFrameRetentionTest, GetFaultServesRetainedFreezeFrameAfterClear) { EXPECT_DOUBLE_EQ(parsed["/ff_pressure"]["data"].get(), 91.25); } +// A rosbag snapshot advertises a download, so the size beside it has to be the size +// of that download. The row keeps the recording's directory total for the storage +// quota. This checks the service reports the served file instead, which is the +// wiring the helper's own unit tests in test_rosbag_capture cannot see. +TEST_F(FaultEventPublishingTest, GetFaultReportsARecordingsServedBytesNotItsFootprint) { + const auto bag_dir = std::filesystem::temp_directory_path() / + ("get_fault_served_" + std::to_string(::getpid()) + "_" + std::to_string(::time(nullptr))); + std::filesystem::create_directories(bag_dir); + + const std::string storage_file = "recording_0.db3"; + { + std::ofstream out(bag_dir / storage_file, std::ios::binary); + out << std::string(8192, 'x'); + } + rosbag2_storage::BagMetadata metadata; + metadata.storage_identifier = "sqlite3"; + metadata.relative_file_paths = {storage_file}; + metadata.duration = std::chrono::nanoseconds(0); + metadata.starting_time = std::chrono::time_point(std::chrono::nanoseconds(0)); + metadata.message_count = 0; + rosbag2_storage::MetadataIo().write_metadata(bag_dir.string(), metadata); + + size_t footprint = 0; + for (const auto & entry : std::filesystem::recursive_directory_iterator(bag_dir)) { + if (entry.is_regular_file()) { + footprint += static_cast(entry.file_size()); + } + } + const auto served = static_cast(std::filesystem::file_size(bag_dir / storage_file)); + ASSERT_GT(footprint, served) << "metadata.yaml did not land, so there is nothing to tell apart"; + + ASSERT_TRUE(call_report_fault("SERVED_BYTES_FAULT", Fault::SEVERITY_ERROR, "/test_node")); + + ros2_medkit_fault_manager::RosbagFileInfo info; + info.fault_code = "SERVED_BYTES_FAULT"; + info.file_path = bag_dir.string(); + info.recording_id = ros2_medkit_fault_manager::rosbag_recording_id(info.file_path); + info.format = "sqlite3"; + info.duration_sec = 5.0; + // What the capture stores: the whole directory, which is what the quota spends. + info.size_bytes = footprint; + info.created_at_ns = 1738664999000000000; + fault_manager_->get_storage_for_test().store_rosbag_file(info); + + auto response = call_get_fault("SERVED_BYTES_FAULT"); + ASSERT_TRUE(response.has_value()); + ASSERT_TRUE(response->success); + + const ros2_medkit_msgs::msg::Snapshot * rosbag_snapshot = nullptr; + for (const auto & snapshot : response->environment_data.snapshots) { + if (snapshot.type == ros2_medkit_msgs::msg::Snapshot::TYPE_ROSBAG) { + rosbag_snapshot = &snapshot; + break; + } + } + ASSERT_NE(rosbag_snapshot, nullptr) << "the stored recording was not reported at all"; + EXPECT_EQ(rosbag_snapshot->size_bytes, served) << "the snapshot must state the bytes a download transfers"; + EXPECT_NE(rosbag_snapshot->size_bytes, footprint) << "the directory total is the quota's figure, not the API's"; + + // The row itself is untouched: the quota still sees the whole recording. + auto row = fault_manager_->get_storage().get_rosbag_file("SERVED_BYTES_FAULT"); + ASSERT_TRUE(row.has_value()); + EXPECT_EQ(row->size_bytes, footprint) << "reporting must not have rewritten what the quota counts"; + + std::error_code ec; + std::filesystem::remove_all(bag_dir, ec); +} + // snapshots.max_per_fault and snapshots.retain_on_clear are independent settings. class UnlimitedSnapshotRetentionTest : public FaultEventPublishingTest { protected: diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp index 92e81667c..e8f25e9b1 100644 --- a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp @@ -32,6 +32,8 @@ #include #include +#include +#include #include #include "rclcpp/rclcpp.hpp" @@ -566,6 +568,153 @@ TEST(RosbagHighBandwidthTopicTest, MatchesSensorStreamsButNotLookalikes) { EXPECT_FALSE(RosbagCapture::is_high_bandwidth_topic("/cmd_vel")); } +// === Reported size vs stored size === +// A recording is stored as a directory and served as a single file. The row keeps the +// directory total, because that is what the recording costs against the storage quota +// (see ABoundaryRecordingSplitsAndReportsTheWholeBag, which pins that). What the API +// reports is the other number: the bytes a download of it transfers. + +namespace { + +/// A bag directory carrying a real ``metadata.yaml``, written by the same library +/// rosbag2 writes it with, so the parse under test is the parse that runs in +/// production rather than a hand-copied literal that can drift from it. +class ServedBytesBag { + public: + explicit ServedBytesBag(const std::string & label) { + dir_ = std::filesystem::temp_directory_path() / + ("served_bytes_" + std::to_string(::getpid()) + "_" + label + "_" + std::to_string(counter_++)); + std::filesystem::create_directories(dir_); + } + + ~ServedBytesBag() { + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + ServedBytesBag(const ServedBytesBag &) = delete; + ServedBytesBag & operator=(const ServedBytesBag &) = delete; + + /// Write a storage file of @p bytes and return its name, relative to the bag. + std::string add_storage_file(const std::string & name, size_t bytes) { + std::ofstream out(dir_ / name, std::ios::binary); + out << std::string(bytes, 'x'); + return name; + } + + void write_metadata(const std::vector & relative_file_paths) { + rosbag2_storage::BagMetadata metadata; + metadata.storage_identifier = "sqlite3"; + metadata.relative_file_paths = relative_file_paths; + metadata.duration = std::chrono::nanoseconds(0); + metadata.starting_time = std::chrono::time_point(std::chrono::nanoseconds(0)); + metadata.message_count = 0; + rosbag2_storage::MetadataIo().write_metadata(dir_.string(), metadata); + } + + /// What the row stores: every regular file under the directory. + size_t directory_total() const { + size_t total = 0; + for (const auto & entry : std::filesystem::recursive_directory_iterator(dir_)) { + if (entry.is_regular_file()) { + total += static_cast(entry.file_size()); + } + } + return total; + } + + size_t file_size_of(const std::string & name) const { + return static_cast(std::filesystem::file_size(dir_ / name)); + } + + const std::filesystem::path & dir() const { + return dir_; + } + std::string path() const { + return dir_.string(); + } + + private: + std::filesystem::path dir_; + static int counter_; +}; + +int ServedBytesBag::counter_ = 0; + +} // namespace + +TEST(RosbagServedBytesTest, ReportsTheStorageFileNotTheDirectoryTotal) { + ServedBytesBag bag("single"); + const std::string db3 = bag.add_storage_file("recording_0.db3", 4096); + bag.write_metadata({db3}); + + const size_t served = bag.file_size_of(db3); + const size_t stored_total = bag.directory_total(); + // Not vacuous: metadata.yaml is on disk too, so the two numbers really differ. + ASSERT_GT(stored_total, served) << "metadata.yaml did not land, so there is nothing to tell apart"; + + EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), served) + << "the reported size must be what a download transfers"; + EXPECT_NE(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), stored_total) + << "metadata.yaml is not served, so it must not be counted"; +} + +TEST(RosbagServedBytesTest, AnUnreadableMetadataFallsBackToTheStoredTotalNotToZero) { + // Positive control on the same harness: with the metadata intact this bag does + // answer with its storage file, so a fallback below is the damaged metadata and + // not a helper that never resolves anything. + ServedBytesBag bag("damaged"); + const std::string db3 = bag.add_storage_file("recording_0.db3", 2048); + bag.write_metadata({db3}); + const size_t stored_total = bag.directory_total(); + ASSERT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), bag.file_size_of(db3)) + << "control: an intact bag resolves its storage file"; + + // Now break only the metadata, leaving the storage file untouched. + { + std::ofstream out(bag.dir() / "metadata.yaml", std::ios::binary | std::ios::trunc); + out << "rosbag2_bagfile_information: [this is not a mapping\n"; + } + const size_t stored_total_after = bag.directory_total(); + EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total_after), stored_total_after) + << "an unparseable metadata.yaml falls back to the stored total"; + EXPECT_NE(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total_after), 0u) + << "and never to zero, which would describe the recording as empty"; +} + +TEST(RosbagServedBytesTest, AMissingMetadataFallsBackToTheStoredTotal) { + // A bag written before metadata was kept, or one whose metadata was lost. + ServedBytesBag bag("nometa"); + bag.add_storage_file("recording_0.db3", 1024); + const size_t stored_total = bag.directory_total(); + + EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), stored_total); +} + +TEST(RosbagServedBytesTest, ANamedFileThatIsNotOnDiskFallsBackToTheStoredTotal) { + ServedBytesBag bag("ghost"); + bag.add_storage_file("recording_0.db3", 1024); + bag.write_metadata({"recording_1.db3"}); // names a segment that was never written + const size_t stored_total = bag.directory_total(); + + EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), stored_total); +} + +TEST(RosbagServedBytesTest, ASplitRecordingFallsBackToTheStoredTotal) { + // Past max_bag_size_mb rosbag2 splits a recording across several storage files. + // The download hands over one of them, so no single file is "the" transfer and the + // recording's own total is the only number that describes it honestly. + ServedBytesBag bag("split"); + const std::string first = bag.add_storage_file("recording_0.db3", 4096); + const std::string second = bag.add_storage_file("recording_1.db3", 2048); + bag.write_metadata({first, second}); + const size_t stored_total = bag.directory_total(); + + const size_t reported = ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total); + EXPECT_EQ(reported, stored_total); + EXPECT_NE(reported, bag.file_size_of(first)) << "picking a segment would advertise a partial recording as whole"; +} + // Fault lifecycle tests TEST_F(RosbagCaptureTest, OnFaultPrefailedWhileDisabled) { From 5263ac95632a8ee27c7f6c2631c59389624fa0fc Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 12:40:05 +0200 Subject: [PATCH 3/6] fix(fault-manager): stop advertising a removed route in the snapshots payload The GetSnapshots response carried rosbag.download_url, built as /api/v1/faults/{code}/snapshots/bag. That route was removed in 0.2.0, so the field gave every caller a URL that answers 404. No code in this repository reads the field. The gateway builds its own entity-scoped bulk-data URI from the recording id it receives on the GetFault snapshot entries. The field is removed, and this payload gets no new URL. A recording is addressed under its entity, as /api/v1/{entity-type}/{id}/bulk-data/rosbags/{recording_id}. Which of the four entity types owns a given source is part of the gateway's discovery model. The fault manager holds the recording id and the reporting source, but it does not hold that mapping. A URL built here would be a guess at one of four prefixes, and a wrong URL that looks right misleads the caller. Service-level tests now cover the size that GetSnapshots, GetRosbag and ListRosbags report, next to the existing GetFault test. A shared bag fixture backs them. The test helpers call file_size() without a cast. It already returns uintmax_t, so the cast was an identity cast that -Wuseless-cast flags. --- .../src/fault_manager_node.cpp | 13 +- .../test/test_fault_manager.cpp | 212 ++++++++++++++---- .../test/test_rosbag_capture.cpp | 4 +- 3 files changed, 184 insertions(+), 45 deletions(-) diff --git a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp index 262ed9cf5..962cdb691 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -1493,7 +1493,17 @@ void FaultManagerNode::handle_get_snapshots( } result["topics"] = topics_json; - // Include rosbag info if available + // Include rosbag info if available. + // + // No download URL. This payload used to carry one built as + // /api/v1/faults/{code}/snapshots/bag, a route that no longer exists and answers + // 404, so the field described a download nobody could perform. It is not replaced + // here either: a recording is addressed under its entity + // (/api/v1/{entity-type}/{id}/bulk-data/rosbags/{recording_id}), and the entity + // type is a fact of the gateway's discovery model that the fault manager does not + // hold. Any URL built here would be a guess at one of four prefixes. The gateway + // resolves the entity itself and builds that URI from the recording id it gets on + // the GetFault snapshot entries, which is the one place the mapping is known. auto rosbag_info = storage_->get_rosbag_file(request->fault_code); if (rosbag_info) { nlohmann::json rosbag_json; @@ -1501,7 +1511,6 @@ void FaultManagerNode::handle_get_snapshots( rosbag_json["duration_sec"] = rosbag_info->duration_sec; rosbag_json["size_bytes"] = rosbag_served_bytes(rosbag_info->file_path, rosbag_info->size_bytes); rosbag_json["format"] = rosbag_info->format; - rosbag_json["download_url"] = "/api/v1/faults/" + request->fault_code + "/snapshots/bag"; result["rosbag"] = rosbag_json; } else { result["rosbag"] = {{"available", false}}; diff --git a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp index 989504c56..d92c53c94 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -42,8 +42,10 @@ #include "ros2_medkit_msgs/msg/snapshot.hpp" #include "ros2_medkit_msgs/srv/clear_fault.hpp" #include "ros2_medkit_msgs/srv/get_fault.hpp" +#include "ros2_medkit_msgs/srv/get_rosbag.hpp" #include "ros2_medkit_msgs/srv/get_snapshots.hpp" #include "ros2_medkit_msgs/srv/list_faults_for_entity.hpp" +#include "ros2_medkit_msgs/srv/list_rosbags.hpp" #include "ros2_medkit_msgs/srv/report_fault.hpp" using ros2_medkit_fault_manager::clamp_debounce_counter; @@ -56,7 +58,10 @@ using ros2_medkit_msgs::msg::Fault; using ros2_medkit_msgs::msg::FaultEvent; using ros2_medkit_msgs::srv::ClearFault; using ros2_medkit_msgs::srv::GetFault; +using ros2_medkit_msgs::srv::GetRosbag; +using ros2_medkit_msgs::srv::GetSnapshots; using ros2_medkit_msgs::srv::ListFaultsForEntity; +using ros2_medkit_msgs::srv::ListRosbags; using ros2_medkit_msgs::srv::ReportFault; /// Default debounce config for tests (matches DebounceConfig defaults: threshold=-1, no healing) @@ -1555,49 +1560,80 @@ TEST_F(FreezeFrameRetentionTest, GetFaultServesRetainedFreezeFrameAfterClear) { EXPECT_DOUBLE_EQ(parsed["/ff_pressure"]["data"].get(), 91.25); } +// === Rosbag reporting through the services === + +namespace { + +/// A bag directory on disk plus the two sizes a recording has: the storage file the +/// download hands over, and the directory total the storage quota is charged. +struct ReportedBag { + explicit ReportedBag(const std::string & label) { + dir = std::filesystem::temp_directory_path() / + ("fm_reported_bag_" + std::to_string(::getpid()) + "_" + label + "_" + std::to_string(counter++)); + std::filesystem::create_directories(dir); + + { + std::ofstream out(dir / storage_file, std::ios::binary); + out << std::string(8192, 'x'); + } + rosbag2_storage::BagMetadata metadata; + metadata.storage_identifier = "sqlite3"; + metadata.relative_file_paths = {storage_file}; + metadata.duration = std::chrono::nanoseconds(0); + metadata.starting_time = std::chrono::time_point(std::chrono::nanoseconds(0)); + metadata.message_count = 0; + rosbag2_storage::MetadataIo().write_metadata(dir.string(), metadata); + + for (const auto & entry : std::filesystem::recursive_directory_iterator(dir)) { + if (entry.is_regular_file()) { + footprint += entry.file_size(); + } + } + served = std::filesystem::file_size(dir / storage_file); + } + + ~ReportedBag() { + std::error_code ec; + std::filesystem::remove_all(dir, ec); + } + + ReportedBag(const ReportedBag &) = delete; + ReportedBag & operator=(const ReportedBag &) = delete; + + /// The row the capture would have stored for @p fault_code: footprint, not served. + ros2_medkit_fault_manager::RosbagFileInfo row_for(const std::string & fault_code) const { + ros2_medkit_fault_manager::RosbagFileInfo info; + info.fault_code = fault_code; + info.file_path = dir.string(); + info.recording_id = ros2_medkit_fault_manager::rosbag_recording_id(info.file_path); + info.format = "sqlite3"; + info.duration_sec = 5.0; + info.size_bytes = footprint; + info.created_at_ns = 1738664999000000000; + return info; + } + + static constexpr const char * storage_file = "recording_0.db3"; + std::filesystem::path dir; + size_t footprint{0}; + size_t served{0}; + static int counter; +}; + +int ReportedBag::counter = 0; + +} // namespace + // A rosbag snapshot advertises a download, so the size beside it has to be the size // of that download. The row keeps the recording's directory total for the storage // quota. This checks the service reports the served file instead, which is the // wiring the helper's own unit tests in test_rosbag_capture cannot see. TEST_F(FaultEventPublishingTest, GetFaultReportsARecordingsServedBytesNotItsFootprint) { - const auto bag_dir = std::filesystem::temp_directory_path() / - ("get_fault_served_" + std::to_string(::getpid()) + "_" + std::to_string(::time(nullptr))); - std::filesystem::create_directories(bag_dir); - - const std::string storage_file = "recording_0.db3"; - { - std::ofstream out(bag_dir / storage_file, std::ios::binary); - out << std::string(8192, 'x'); - } - rosbag2_storage::BagMetadata metadata; - metadata.storage_identifier = "sqlite3"; - metadata.relative_file_paths = {storage_file}; - metadata.duration = std::chrono::nanoseconds(0); - metadata.starting_time = std::chrono::time_point(std::chrono::nanoseconds(0)); - metadata.message_count = 0; - rosbag2_storage::MetadataIo().write_metadata(bag_dir.string(), metadata); - - size_t footprint = 0; - for (const auto & entry : std::filesystem::recursive_directory_iterator(bag_dir)) { - if (entry.is_regular_file()) { - footprint += static_cast(entry.file_size()); - } - } - const auto served = static_cast(std::filesystem::file_size(bag_dir / storage_file)); - ASSERT_GT(footprint, served) << "metadata.yaml did not land, so there is nothing to tell apart"; + ReportedBag bag("get_fault"); + ASSERT_GT(bag.footprint, bag.served) << "metadata.yaml did not land, so there is nothing to tell apart"; ASSERT_TRUE(call_report_fault("SERVED_BYTES_FAULT", Fault::SEVERITY_ERROR, "/test_node")); - - ros2_medkit_fault_manager::RosbagFileInfo info; - info.fault_code = "SERVED_BYTES_FAULT"; - info.file_path = bag_dir.string(); - info.recording_id = ros2_medkit_fault_manager::rosbag_recording_id(info.file_path); - info.format = "sqlite3"; - info.duration_sec = 5.0; - // What the capture stores: the whole directory, which is what the quota spends. - info.size_bytes = footprint; - info.created_at_ns = 1738664999000000000; - fault_manager_->get_storage_for_test().store_rosbag_file(info); + fault_manager_->get_storage_for_test().store_rosbag_file(bag.row_for("SERVED_BYTES_FAULT")); auto response = call_get_fault("SERVED_BYTES_FAULT"); ASSERT_TRUE(response.has_value()); @@ -1611,16 +1647,110 @@ TEST_F(FaultEventPublishingTest, GetFaultReportsARecordingsServedBytesNotItsFoot } } ASSERT_NE(rosbag_snapshot, nullptr) << "the stored recording was not reported at all"; - EXPECT_EQ(rosbag_snapshot->size_bytes, served) << "the snapshot must state the bytes a download transfers"; - EXPECT_NE(rosbag_snapshot->size_bytes, footprint) << "the directory total is the quota's figure, not the API's"; + EXPECT_EQ(rosbag_snapshot->size_bytes, bag.served) << "the snapshot must state the bytes a download transfers"; + EXPECT_NE(rosbag_snapshot->size_bytes, bag.footprint) << "the directory total is the quota's figure, not the API's"; // The row itself is untouched: the quota still sees the whole recording. auto row = fault_manager_->get_storage().get_rosbag_file("SERVED_BYTES_FAULT"); ASSERT_TRUE(row.has_value()); - EXPECT_EQ(row->size_bytes, footprint) << "reporting must not have rewritten what the quota counts"; + EXPECT_EQ(row->size_bytes, bag.footprint) << "reporting must not have rewritten what the quota counts"; +} - std::error_code ec; - std::filesystem::remove_all(bag_dir, ec); +// The other three services that quote a recording's size. GetFault above covers the +// snapshot entry. These are the remaining answers, each reached through its own +// service call rather than through the helper. +TEST_F(FaultEventPublishingTest, EveryRosbagServiceReportsTheServedBytes) { + ReportedBag bag("all_services"); + ASSERT_GT(bag.footprint, bag.served) << "metadata.yaml did not land, so there is nothing to tell apart"; + + ASSERT_TRUE(call_report_fault("ALL_SERVICES_FAULT", Fault::SEVERITY_ERROR, "/test_node")); + const auto row = bag.row_for("ALL_SERVICES_FAULT"); + fault_manager_->get_storage_for_test().store_rosbag_file(row); + + const std::string ns = test_node_->get_namespace(); + + // GetSnapshots: the rosbag block of the JSON payload. + { + auto client = test_node_->create_client(ns + "/fault_manager/get_snapshots"); + ASSERT_TRUE(client->wait_for_service(std::chrono::seconds(5))); + auto request = std::make_shared(); + request->fault_code = "ALL_SERVICES_FAULT"; + auto future = client->async_send_request(request); + ASSERT_TRUE(spin_until_future_ready(future)); + auto response = future.get(); + ASSERT_TRUE(response->success) << response->error_message; + + auto payload = nlohmann::json::parse(response->data); + ASSERT_TRUE(payload.contains("rosbag")); + ASSERT_TRUE(payload["rosbag"].value("available", false)); + EXPECT_EQ(payload["rosbag"]["size_bytes"].get(), bag.served); + EXPECT_NE(payload["rosbag"]["size_bytes"].get(), bag.footprint); + } + + // GetRosbag: the single-recording lookup. + { + auto client = test_node_->create_client(ns + "/fault_manager/get_rosbag"); + ASSERT_TRUE(client->wait_for_service(std::chrono::seconds(5))); + auto request = std::make_shared(); + request->recording_id = row.recording_id; + request->fault_code = "ALL_SERVICES_FAULT"; + auto future = client->async_send_request(request); + ASSERT_TRUE(spin_until_future_ready(future)); + auto response = future.get(); + ASSERT_TRUE(response->success) << response->error_message; + EXPECT_EQ(response->size_bytes, bag.served); + EXPECT_NE(response->size_bytes, bag.footprint); + } + + // ListRosbags: the per-entity listing, keyed by the fault's reporting source. + { + auto client = test_node_->create_client(ns + "/fault_manager/list_rosbags"); + ASSERT_TRUE(client->wait_for_service(std::chrono::seconds(5))); + auto request = std::make_shared(); + request->entity_fqn = "/test_node"; + auto future = client->async_send_request(request); + ASSERT_TRUE(spin_until_future_ready(future)); + auto response = future.get(); + ASSERT_TRUE(response->success) << response->error_message; + ASSERT_EQ(response->sizes_bytes.size(), 1u) << "the stored recording was not listed"; + EXPECT_EQ(response->sizes_bytes[0], bag.served); + EXPECT_NE(response->sizes_bytes[0], bag.footprint); + } +} + +// The legacy /faults/{code}/snapshots/bag route was removed, so a payload naming it +// hands the caller a 404. Nothing in this repo reads the field, and a recording is +// addressed under its entity, which the fault manager cannot resolve, so the field +// is gone rather than repointed. +TEST_F(FaultEventPublishingTest, GetSnapshotsDoesNotAdvertiseARouteThatWasRemoved) { + ReportedBag bag("no_download_url"); + + ASSERT_TRUE(call_report_fault("NO_URL_FAULT", Fault::SEVERITY_ERROR, "/test_node")); + fault_manager_->get_storage_for_test().store_rosbag_file(bag.row_for("NO_URL_FAULT")); + + auto client = test_node_->create_client(std::string(test_node_->get_namespace()) + + "/fault_manager/get_snapshots"); + ASSERT_TRUE(client->wait_for_service(std::chrono::seconds(5))); + auto request = std::make_shared(); + request->fault_code = "NO_URL_FAULT"; + auto future = client->async_send_request(request); + ASSERT_TRUE(spin_until_future_ready(future)); + auto response = future.get(); + ASSERT_TRUE(response->success) << response->error_message; + + auto payload = nlohmann::json::parse(response->data); + + // Positive control for the absence below: the rosbag block IS present and populated, + // so a missing key is a dropped field and not an empty or absent payload. + ASSERT_TRUE(payload.contains("rosbag")) << "control: the payload carries a rosbag block"; + ASSERT_TRUE(payload["rosbag"].value("available", false)) << "control: the recording was found"; + ASSERT_TRUE(payload["rosbag"].contains("format")) << "control: the block still carries its other fields"; + + EXPECT_FALSE(payload["rosbag"].contains("download_url")) + << "the payload advertises a route that answers 404: " << payload["rosbag"].dump(); + // Nowhere else in the payload either. + EXPECT_EQ(payload.dump().find("snapshots/bag"), std::string::npos) + << "a removed route is named somewhere in the payload: " << payload.dump(); } // snapshots.max_per_fault and snapshots.retain_on_clear are independent settings. diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp index e8f25e9b1..d95d61fc6 100644 --- a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp @@ -617,14 +617,14 @@ class ServedBytesBag { size_t total = 0; for (const auto & entry : std::filesystem::recursive_directory_iterator(dir_)) { if (entry.is_regular_file()) { - total += static_cast(entry.file_size()); + total += entry.file_size(); } } return total; } size_t file_size_of(const std::string & name) const { - return static_cast(std::filesystem::file_size(dir_ / name)); + return std::filesystem::file_size(dir_ / name); } const std::filesystem::path & dir() const { From e5eed0219daf7f93e1c5201e15644bbe0ce57d26 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 13:51:33 +0200 Subject: [PATCH 4/6] fix(gateway): size a rosbag by its named storage file and survive an unreadable bag Sizing rule. For a bag directory, the listing measures a file only when metadata.yaml names exactly one storage file, the name is a direct child of the bag with a .db3 or .mcap extension, and the file is present. In every other case the descriptor keeps the row's figure, which is the fault manager's answer for the same recording. That covers a split recording, a missing or unparsable metadata.yaml, a name that escapes the directory, and a named file that is gone. The fault manager decides "split" from the same relative_file_paths field, so both API surfaces report one size for one recording. A path that is itself a storage file is one storage file, and the listing sizes it as it is. A split recording keeps its total because the download hands over one segment, and no single file describes the transfer. The descriptor size is then larger than the Content-Length of the download. rest.rst states this as the way a client recognises a partial download of a split recording. The Content-Length entry links to that rule with explicit link text, because the label sits on a paragraph. Resolver. When the metadata names one storage file under the same rule and the file exists, the resolver returns that file. Otherwise it scans the directory for the first .db3 or .mcap, so the download always has a file to hand over when the directory holds one. A stray file next to the recording, such as a leftover segment or a copy, is never sized as the recording. The gateway reads relative_file_paths with yaml-cpp, which it already links. It does not link rosbag2_storage. Filesystem errors. Every filesystem call in the resolver and in the size helper uses the std::error_code overload. EACCES on one bag directory, or ENOENT when quota eviction removes a bag during the walk, now leaves that one recording unmeasured. The listing handler has no catch in its chain, so the exception answered 500 and dropped every other recording of the entity. The download route shares the resolver and gets the same protection. The comments on the resolver and on rosbag_served_bytes state when the listing and the download report the same length. README. The download is one storage file streamed verbatim, named . (.mcap or .sqlite3). It is not an archive and it does not include metadata.yaml. The playback section runs ros2 bag info and ros2 bag play on the downloaded file, with no unpacking step and no --storage flag, for both storage formats. The Postman collection drops its three requests to the snapshot routes that were removed in 0.2.0. The collection has no bulk-data section to move them to. Tests. A new integration test compares a descriptor's size with the bytes its own download delivers and with Content-Length. Unit tests cover a split recording, an unreadable directory, a bare storage file, a stray file, a named file that is gone, a name that climbs out of the bag, an absolute name, a name in a subdirectory and a name without a storage extension. The stray-file test names the storage file that the directory scan would not reach first. The mode-0000 test drops CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH from its own thread's effective set when it runs as root. It restores them through a guard object, and it asserts that the permission is denied before it asserts anything about the resolver. A test uses the result of a nodiscard is_directory call. A fault manager test pins the same 4096-byte answer for a named recording with a stray file beside it. --- docs/api/rest.rst | 17 +- ...os2-medkit-gateway.postman_collection.json | 68 -- .../test/test_rosbag_capture.cpp | 22 + src/ros2_medkit_gateway/README.md | 36 +- .../core/http/handlers/bulkdata_handlers.hpp | 65 +- .../src/http/handlers/bulkdata_handlers.cpp | 218 ++++++- .../test/test_bulkdata_handlers.cpp | 581 +++++++++++++++++- .../test/features/test_bulk_data_api.test.py | 45 ++ 8 files changed, 928 insertions(+), 124 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 762416032..47e308823 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1774,9 +1774,10 @@ is the time that recording was made. ``size`` is the number of bytes the download route below puts on the wire for that descriptor, so a client can size a buffer or a progress bar from the -listing. For a rosbag that is the bag's single storage file (``.mcap`` or -``.db3``), which is the only file the download serves. The bag directory also -holds ``metadata.yaml``, and those bytes are not part of the transfer. +listing. For a rosbag held in a single storage file, which is the normal case, +that is the file (``.mcap`` or ``.sqlite3``) and it is the only file the +download serves. The bag directory also holds ``metadata.yaml``, and those bytes +are not part of the transfer. .. _rest-recording-size-rule: @@ -1791,6 +1792,13 @@ split across several storage files, past the configured maximum bag size: the download can hand over only one of them, no single file describes the transfer, and the API reports the recording's total instead. +For that split case the three numbers stop agreeing, and deliberately so. The +descriptor ``size`` and the nested ``size_bytes`` report the recording's total +while the download's ``Content-Length`` is the one storage file it hands over, +so ``size`` exceeds ``Content-Length``. That gap is the signal: a client that +compares the two can tell the transfer it just made is a part of the recording +rather than the whole of it, which no single reported number could express. + Download Bulk Data ~~~~~~~~~~~~~~~~~~ @@ -1806,6 +1814,9 @@ Download a specific bulk-data file. a pre-#620 fault-code URL is not the segment the client sent, and the format is the one persisted at capture time (``mcap`` or ``sqlite3``). For every other category it is the stored item's own name, e.g. ``report.zip``. +- ``Content-Length``: the served file's length. For how it relates to the + descriptor ``size`` of the same recording, see + :ref:`One recording, one size ` - ``Accept-Ranges``: ``bytes`` - the download is served by a range-aware provider, so a client may fetch part of the file - ``Access-Control-Expose-Headers``: ``Content-Disposition`` diff --git a/postman/collections/ros2-medkit-gateway.postman_collection.json b/postman/collections/ros2-medkit-gateway.postman_collection.json index 5a138a0da..6a3b69f17 100644 --- a/postman/collections/ros2-medkit-gateway.postman_collection.json +++ b/postman/collections/ros2-medkit-gateway.postman_collection.json @@ -1355,74 +1355,6 @@ "description": "List faults with cluster details. Clusters are groups of similar faults that occurred within a time window.\n\nEach cluster includes:\n- `cluster_id`: Unique cluster identifier\n- `rule_id`, `rule_name`: The auto-cluster rule that created this cluster\n- `representative_code`: The fault shown as the cluster representative (based on rule config: first, most_recent, or highest_severity)\n- `representative_severity`: Severity of the representative fault\n- `fault_codes[]`: All fault codes in the cluster\n- `first_at`, `last_at`: Timestamps of first and last faults in cluster" }, "response": [] - }, - { - "name": "GET Fault Snapshots (System-wide)", - "request": { - "method": "GET", - "header": [], - "url": { - "raw": "{{base_url}}/faults/SENSOR_OVERTEMP/snapshots", - "host": [ - "{{base_url}}" - ], - "path": [ - "faults", - "SENSOR_OVERTEMP", - "snapshots" - ] - }, - "description": "Get topic snapshots captured when a fault transitioned to CONFIRMED status. Snapshots provide system state at the moment of fault confirmation for post-mortem debugging. Returns object with fault_code, captured_at timestamp, and topics object keyed by topic name containing message_type and parsed data." - }, - "response": [] - }, - { - "name": "GET Fault Snapshots (Filtered by Topic)", - "request": { - "method": "GET", - "header": [], - "url": { - "raw": "{{base_url}}/faults/SENSOR_OVERTEMP/snapshots?topic=/joint_states", - "host": [ - "{{base_url}}" - ], - "path": [ - "faults", - "SENSOR_OVERTEMP", - "snapshots" - ], - "query": [ - { - "key": "topic", - "value": "/joint_states" - } - ] - }, - "description": "Get snapshots filtered by specific topic name. Use this when you only need data from a particular topic." - }, - "response": [] - }, - { - "name": "GET Component Fault Snapshots", - "request": { - "method": "GET", - "header": [], - "url": { - "raw": "{{base_url}}/components/temp_sensor/faults/SENSOR_OVERTEMP/snapshots", - "host": [ - "{{base_url}}" - ], - "path": [ - "components", - "temp_sensor", - "faults", - "SENSOR_OVERTEMP", - "snapshots" - ] - }, - "description": "Get topic snapshots for a specific component's fault. Same as the system-wide endpoint but scoped to a component context. Useful when working within a component-centric workflow." - }, - "response": [] } ] }, diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp index d95d61fc6..c8c75f149 100644 --- a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp @@ -700,6 +700,28 @@ TEST(RosbagServedBytesTest, ANamedFileThatIsNotOnDiskFallsBackToTheStoredTotal) EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), stored_total); } +TEST(RosbagServedBytesTest, ASingleNamedFileIsSizedEvenWithAStrayFileBesideIt) { + // A leftover segment or a copy sitting beside the recording must not change + // which file is measured. The metadata names one file and that is the file. + // + // This is also the fault manager's half of a cross-package agreement: the + // gateway sizes the same directory shape through its own resolver, and its + // TheMetadataNamesTheStorageFileRatherThanDirectoryOrder asserts the same + // 4096. Directory order decided the gateway's answer until it read this field + // too, and the two sides reported different sizes for one recording. + ServedBytesBag bag("stray"); + const std::string named = bag.add_storage_file("recording_0.db3", 4096); + bag.add_storage_file("recording_1.db3", 65536); + bag.write_metadata({named}); + const size_t stored_total = bag.directory_total(); + + const size_t reported = ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total); + EXPECT_EQ(reported, bag.file_size_of(named)); + EXPECT_EQ(reported, 4096u) << "the same number the gateway's listing reports for this shape"; + EXPECT_NE(reported, bag.file_size_of("recording_1.db3")) << "the stray file is not the recording"; + EXPECT_NE(reported, stored_total); +} + TEST(RosbagServedBytesTest, ASplitRecordingFallsBackToTheStoredTotal) { // Past max_bag_size_mb rosbag2 splits a recording across several storage files. // The download hands over one of them, so no single file is "the" transfer and the diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index decd82459..8227c874f 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1158,8 +1158,17 @@ confirmation. A recording is listed and downloaded through the bulk-data endpoints: `GET /api/v1/{entity-path}/bulk-data/rosbags` for the descriptors and `GET /api/v1/{entity-path}/bulk-data/rosbags/{recording_id}` for the bytes. The -download serves the bag's single storage file (`.mcap` or `.db3`), and the -descriptor `size` is that file's length. +download serves one storage file, verbatim, named `.` +(`.mcap` or `.sqlite3`). It is not an archive and it does not include the bag's +`metadata.yaml`. + +For a recording held in a single storage file, which is the normal case, the +descriptor `size` is that file's length and therefore the length of the +download. A recording that grew past `snapshots.rosbag.max_bag_size_mb` is split +across several storage files and the download hands over only one of them. The +descriptor then reports the recording's total, so `size` exceeds the download's +`Content-Length`. See [the size rule](../../docs/api/rest.rst) in the REST API +reference for the full statement. **Rosbag Configuration:** @@ -1187,17 +1196,28 @@ ros2 run ros2_medkit_fault_manager fault_manager_node \ ``` **Playback downloaded rosbag:** + +The downloaded file is a bag in itself. Point `ros2 bag` straight at it, with no +unpacking step and no `--storage` flag - rosbag2 reads the storage id out of the +file, so the same two commands work for `.mcap` and for `.sqlite3`. + ```bash -# Extract the downloaded archive -tar -xzf fault_MOTOR_OVERHEAT_20260124_153045.tar.gz +# Inspect the downloaded file +ros2 bag info fault_MOTOR_OVERHEAT_1738664999000.mcap -# Play back the bag -ros2 bag play fault_MOTOR_OVERHEAT_1735830000/ +# Play it back +ros2 bag play fault_MOTOR_OVERHEAT_1738664999000.mcap +``` -# Inspect bag contents -ros2 bag info fault_MOTOR_OVERHEAT_1735830000/ +```bash +# The same, for a recording captured with snapshots.rosbag.format: sqlite3 +ros2 bag info fault_MOTOR_OVERHEAT_1738664999000.sqlite3 +ros2 bag play fault_MOTOR_OVERHEAT_1738664999000.sqlite3 ``` +A lone storage file needs no `metadata.yaml` beside it: both commands read the +topics, the message count and the duration out of the file itself. + **Differences from JSON Snapshots:** | Feature | JSON Snapshots | Rosbag Capture | diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp index f61b03f1d..80f6ec0f0 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp @@ -108,13 +108,26 @@ class BulkDataHandlers { * @brief Resolve rosbag file path from storage path. * * Rosbag2 creates a directory containing the actual db3/mcap file. - * This function resolves the directory to the actual file path. + * This function resolves the directory to the actual file path. A path that is + * already a regular file is returned unchanged. * - * The single place that decides which bytes a recording IS. `download()` - * streams the file this returns and reports its length. The listing sizes - * its descriptor from the same file through `detail::rosbag_served_bytes`. - * Both must move together, which is why this is reachable from outside the - * class rather than a private helper of the download path. + * The single place that decides which bytes a recording IS, which is why it is + * reachable from outside the class. `download()` streams the file this returns + * and reports its length, and `detail::rosbag_served_bytes` sizes the listing + * through it, so a change to which file a recording resolves to moves both at + * once. + * + * That does not make the two numbers equal in every case, and since the split + * fix it deliberately does not. For a recording held in one storage file the + * listing resolves through here and reports exactly what the download sends. + * For a recording split across several files the listing does not come through + * here at all: it carries the recording's total from the fault manager while + * this route still hands over one file, and the gap is what tells a client the + * transfer is partial. See the size rule in ``docs/api/rest.rst``. + * + * When the bag's own ``metadata.yaml`` names exactly one storage file and that + * file exists, that is the file. Otherwise the directory is scanned for the + * first ``.db3`` or ``.mcap`` in whatever order it yields. * * @param path Path to rosbag (can be file or directory) * @return Resolved file path, or empty string if not found @@ -216,21 +229,35 @@ bool rosbag_resolved_by_fault_code(const nlohmann::json & rosbag_data, const std /** * @brief Bytes a rosbag download puts on the wire for one recording. * - * ``BulkDataHandlers::resolve_rosbag_file_path`` picks the single storage file - * inside the bag directory and the download streams that file alone, so the - * length a client is told to expect is that file's length and nothing else. + * Answers only for a recording held in a single storage file, which is the only + * shape where one number describes the transfer. That is a bag directory whose + * ``metadata.yaml`` names one file, or a @p bag_path that is itself a storage + * file, which is one by definition and carries no metadata to consult. + * ``BulkDataHandlers::resolve_rosbag_file_path`` picks that file and the + * download streams it alone, so the length a client is told to expect is that + * file's length and nothing else. Reporting the bag directory's total instead + * overstated every download by ``metadata.yaml`` - on a short recording, by + * around a tenth of the transfer - and a client sizing a buffer or a progress + * bar from the listing never reached the end. + * + * Returns nullopt for anything else, and the caller then keeps the row's own + * figure. That covers a bag this process cannot see or read at all, and it + * covers a recording split across several storage files past the configured + * maximum bag size: the download hands over one segment, so no single file is + * the transfer, and answering with whichever segment the resolver reached first + * advertised a split recording at the size of one part of it. The row's figure + * is the fault manager's answer to the same question, decided from the same + * ``metadata.yaml``, so deferring to it keeps the two API surfaces agreeing on + * one recording. * - * The fault manager's stored ``size_bytes`` answers a different question. It - * walks the whole bag directory, because it is the figure the recording's disk - * quota is spent against, and the directory also holds ``metadata.yaml``. - * Reporting that figure as the descriptor size overstated every download by the - * metadata file - on a short recording, by around a tenth of the transfer - and - * a client sizing a buffer or a progress bar from the listing never reached the - * end. The listing therefore states what the download serves, measured on the - * file the download resolves, and leaves the quota figure to the quota. + * Never throws and never reports a filesystem error upwards. It runs once per + * row of a listing, and an error here is one unreadable recording, not a failed + * request for the entity's other recordings. * - * @param bag_path Bag path as stored by the fault manager (directory or file) - * @return The resolved file's size, or nullopt when this process cannot see it + * @param bag_path Bag path as stored by the fault manager. A bag directory, or + * a bare storage file, which both answer + * @return The single storage file's size, or nullopt when there is not exactly + * one, or when this process cannot see it */ std::optional rosbag_served_bytes(const std::string & bag_path); diff --git a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp index 26bced8e5..a2aa17180 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -17,6 +17,7 @@ #include "ros2_medkit_gateway/core/faults/fault_scope.hpp" #include +#include #include #include #include @@ -31,6 +32,7 @@ #include #include +#include #include "ros2_medkit_gateway/core/http/entity_path_utils.hpp" #include "ros2_medkit_gateway/core/http/error_codes.hpp" @@ -63,6 +65,85 @@ tl::expected parse_path(const http::TypedRequest & re return *info; } +/// The storage files the bag at @p bag_path recorded, named by its own +/// ``metadata.yaml`` in ``relative_file_paths``. +/// +/// This is the bag's own record of what it contains, and it is the same field the +/// fault manager reads for the same decisions. The same YAML parser underneath, +/// two decoders: ``rosbag2_storage::MetadataIo`` decodes the whole document into a +/// ``BagMetadata``, this reads one sequence out of it. The two agree for every +/// document both decoders accept, which is what keeps the sides describing one +/// recording alike. The gateway already links yaml-cpp, so this costs no new +/// dependency, and it does not link rosbag2_storage, which is why the field is +/// read here directly. +/// +/// nullopt when the metadata is missing, unreadable, or not the shape rosbag2 +/// writes - all of which mean the same thing to a caller, that the bag will not +/// say and the directory has to be inspected instead. +std::optional> rosbag_relative_file_paths(const std::string & bag_path) { + const std::filesystem::path metadata_path = std::filesystem::path(bag_path) / "metadata.yaml"; + std::error_code ec; + if (!std::filesystem::is_regular_file(metadata_path, ec)) { + return std::nullopt; + } + try { + const YAML::Node root = YAML::LoadFile(metadata_path.string()); + if (!root.IsMap()) { + return std::nullopt; + } + const YAML::Node info = root["rosbag2_bagfile_information"]; + if (!info || !info.IsMap()) { + return std::nullopt; + } + const YAML::Node paths = info["relative_file_paths"]; + if (!paths || !paths.IsSequence()) { + return std::nullopt; + } + std::vector names; + names.reserve(paths.size()); + for (const auto & entry : paths) { + if (!entry.IsScalar()) { + return std::nullopt; + } + names.push_back(entry.as()); + } + return names; + } catch (const std::exception &) { + return std::nullopt; + } +} + +/// The file @p relative_name names inside the bag at @p bag_path, when that is +/// a name this side will follow: a direct child of the bag directory carrying a +/// storage extension, which is what rosbag2 writes. +/// +/// The containment test is the point. ``std::filesystem::path`` concatenation +/// lets an absolute name replace the directory outright (``bag / "/etc/passwd"`` +/// is ``/etc/passwd``) and a ``..`` name climb out of it, so without this a +/// metadata.yaml would choose which file on the host the download hands over +/// and advertises a length for. The extension test keeps the same rule the +/// directory scan applies to every other candidate. +/// +/// nullopt when the name is not one of those. Callers treat that exactly as a +/// bag that named nothing: the resolver scans the directory, the size helper +/// declines. +std::optional rosbag_named_storage_file(const std::string & bag_path, + const std::string & relative_name) { + std::filesystem::path bag = std::filesystem::path(bag_path).lexically_normal(); + if (!bag.has_filename()) { + bag = bag.parent_path(); // tolerate a trailing slash + } + const std::filesystem::path named = (bag / relative_name).lexically_normal(); + if (named.parent_path() != bag) { + return std::nullopt; + } + const std::string ext = named.extension().string(); + if (ext != ".db3" && ext != ".mcap") { + return std::nullopt; + } + return named; +} + } // namespace BulkDataHandlers::BulkDataHandlers(HandlerContext & ctx) : ctx_(ctx) { @@ -87,24 +168,67 @@ std::vector BulkDataHandlers::download_media_types() { } std::string BulkDataHandlers::resolve_rosbag_file_path(const std::string & path) { + // Every filesystem call below takes the std::error_code overload, and that is + // load-bearing. This runs once per row of a bulk-data + // listing. With the throwing overloads one unreadable bag directory (EACCES), + // or one removed by quota eviction between the is_directory test and the walk + // (ENOENT), threw out of list(), which has no catch anywhere in its chain, and + // the request answered 500: a single bag nobody could read took every other + // recording of that entity out of the listing with it. Here a bag this process + // cannot read is an empty answer, not a failed request. + std::error_code ec; + // If it's a regular file, return as-is - if (std::filesystem::is_regular_file(path)) { + if (std::filesystem::is_regular_file(path, ec)) { return path; } // If it's a directory (rosbag2 directory structure), find the db3/mcap file inside - if (std::filesystem::is_directory(path)) { - for (const auto & entry : std::filesystem::directory_iterator(path)) { - if (entry.is_regular_file()) { - auto ext = entry.path().extension().string(); - // Look for db3 (sqlite3 format) or mcap files - if (ext == ".db3" || ext == ".mcap") { - return entry.path().string(); - } + if (!std::filesystem::is_directory(path, ec)) { + return ""; + } + + // Ask the bag first. When its metadata names exactly one storage file and that + // file is there, that is the file, and directory order does not get a vote. + // Iterating instead returns whichever .db3 or .mcap the directory happens to + // yield first, so a stray file beside the recording - a leftover segment, a + // copy - can be served and sized in place of the real one, while the fault + // manager, which sizes the single file it names (a split falls back to the + // stored total), reports the other. Both sides read the same field. + // + // Several names is a split recording and is deliberately left to the loop + // below: the download's choice of segment there is a separate question from + // this one. No metadata, unreadable metadata, a name this side will not follow + // and a named file that is not on disk all fall through as well, because then + // the bag has not answered. + if (const auto names = rosbag_relative_file_paths(path); names && names->size() == 1) { + if (const auto named = rosbag_named_storage_file(path, names->front())) { + std::error_code named_ec; + if (std::filesystem::is_regular_file(*named, named_ec) && !named_ec) { + return named->string(); } } } + std::filesystem::directory_iterator it(path, ec); + if (ec) { + return ""; + } + for (const std::filesystem::directory_iterator end; it != end; it.increment(ec)) { + if (ec) { + return ""; + } + std::error_code entry_ec; + if (!it->is_regular_file(entry_ec) || entry_ec) { + continue; + } + auto ext = it->path().extension().string(); + // Look for db3 (sqlite3 format) or mcap files + if (ext == ".db3" || ext == ".mcap") { + return it->path().string(); + } + } + return ""; // File not found } @@ -126,6 +250,40 @@ std::optional rosbag_served_bytes(const std::string & bag_path) { if (bag_path.empty()) { return std::nullopt; } + + std::error_code path_ec; + const bool is_storage_file = std::filesystem::is_regular_file(bag_path, path_ec) && !path_ec; + + // Only a recording held in a single storage file has a size the download can + // be measured by. Past the configured maximum bag size rosbag2 splits a + // recording across several files and the download route hands over one of + // them, so no single file is "the" transfer, and the fault manager reports the + // recording's total for that shape. On nullopt the descriptor keeps the row's + // figure, which since the fault manager began sending served bytes IS that + // answer: the storage file for a whole recording, the directory total for a + // split one. + // + // Which file a single-file recording is held in comes from the bag's own + // metadata, and the size follows that file and no other. A named file that is + // absent leaves no answer here, the shape the fault manager answers with the + // stored total for. The resolver falls through to directory order there so the + // download still has something to hand over; that fallback does not make a + // stray file the recording's size. + // + // A bag_path that is itself a regular file IS the one storage file, and asking + // its metadata is meaningless because a file has no metadata.yaml beside it + // under that name. The path can be one: the resolver accepts a bare file and + // the download serves it, so the count gate sits behind the is_storage_file + // test - in front of it, every such row would be declined here and listed at + // zero. + std::optional> names; + if (!is_storage_file) { + names = rosbag_relative_file_paths(bag_path); + if (!names || names->size() != 1) { + return std::nullopt; + } + } + // The same two steps `download()` performs, in the same order and through the // same resolver, so the size a client is promised cannot drift from the size // it is sent. Changing which file a recording resolves to changes both. @@ -133,6 +291,17 @@ std::optional rosbag_served_bytes(const std::string & bag_path) { if (resolved.empty()) { return std::nullopt; } + + // The measurement stands only when the file resolved IS the file the bag + // named, through the same containment rule the resolver applied, so the two + // cannot disagree about which names are followable. + if (!is_storage_file) { + const auto named = rosbag_named_storage_file(bag_path, names->front()); + if (!named || std::filesystem::path(resolved).lexically_normal() != *named) { + return std::nullopt; + } + } + std::error_code ec; const auto size = std::filesystem::file_size(resolved, ec); if (ec) { @@ -226,13 +395,15 @@ fold_rosbag_rows_into_descriptors(const std::vector & rows, // Default to sqlite3 (the historical FaultManager default) when a bag predates // the persisted format field; the per-bag metadata normally carries the real one. entry.format = row.value("format", "sqlite3"); - // What the download route will actually send, measured on the file it - // resolves. The stored figure is the bag directory's total, which is the - // recording's footprint against the disk quota and not its transfer size - - // it counts metadata.yaml, which the download does not serve. Keep the - // stored figure only when the bag is not visible from this process: it is - // then the only number available, and listing a zero would describe the - // recording as empty rather than as unmeasured. + // What the download route will actually send, measured here on the file it + // resolves. The row's figure is the fault manager's own answer to the same + // question: the storage file for a recording held in one file, the bag + // directory's total for one split across several, and the total again for a + // bag whose metadata it could not read. Measuring locally is what keeps the + // listing and the download from drifting apart on this host. Falling back to + // the row is what keeps a recording this process cannot see - a peer's bag, + // an unreadable directory, a split - described by the side that can. A zero in + // its place would describe the recording as empty. entry.size_bytes = rosbag_served_bytes(row.value("file_path", "")).value_or(row.value("size_bytes", uint64_t{0})); entry.duration_sec = row.value("duration_sec", 0.0); entry.created_at_ns = created_at_ns; @@ -519,9 +690,18 @@ http::Result BulkDataHandlers::download(const http::TypedR filename = rosbag_result.data.value("recording_id", bulk_data_id) + "." + format; // Rosbag2 emits a directory layout - resolve the inner db3/mcap file. Only - // that file is served, and metadata.yaml stays on the gateway host. The listing - // sizes its descriptor through detail::rosbag_served_bytes, which resolves - // the same way, so the Content-Length below is the number it advertised. + // that file is served, and metadata.yaml stays on the gateway host. + // + // For a recording whose metadata names its single storage file and that file + // is present, which is the normal case, the listing resolved this same path + // through detail::rosbag_served_bytes and the Content-Length below is the + // number it advertised. When the bag names no file this side will follow - + // no metadata, a split, a name that escapes the directory, a named file that + // is gone - the descriptor carries the total the fault manager stored while + // this route still hands over whatever the directory holds, so the two + // deliberately differ and the descriptor size exceeding Content-Length is how + // a client can tell the transfer is not the whole recording. See the size + // rule in docs/api/rest.rst. actual_path = resolve_rosbag_file_path(file_path); } else { // === Non-rosbag categories: served via BulkDataStore === diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index d3ba1048b..83236e1f9 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -15,14 +15,19 @@ #include #include +#include #include +#include #include #include +#include #include #include #include +#include #include #include +#include #include #include "ros2_medkit_gateway/core/discovery/models/app.hpp" @@ -267,7 +272,7 @@ class RosbagBagDirectoryTest : public ::testing::Test { ("bulkdata_bag_test_" + std::to_string(getpid()) + "_" + std::to_string(counter_++)); std::filesystem::create_directories(bag_dir_); write_file(bag_dir_ / "recording_0.db3", std::string(4096, 'x')); - write_file(bag_dir_ / "metadata.yaml", std::string(311, 'y')); + write_metadata(bag_dir_, {"recording_0.db3"}); } void TearDown() override { @@ -280,12 +285,31 @@ class RosbagBagDirectoryTest : public ::testing::Test { out << content; } + /// A ``metadata.yaml`` in the shape rosbag2 writes, naming @p storage_files in + /// ``relative_file_paths``. Only the fields this code reads are filled in, but + /// the nesting is the real one: the helper looks up + /// ``rosbag2_bagfile_information.relative_file_paths``, so a flat document + /// would pass a test that production data fails. + static void write_metadata(const std::filesystem::path & dir, const std::vector & storage_files) { + std::string yaml = + "rosbag2_bagfile_information:\n" + " version: 9\n" + " storage_identifier: sqlite3\n" + " message_count: 0\n" + " relative_file_paths:\n"; + for (const auto & file : storage_files) { + yaml += " - " + file + "\n"; + } + yaml += " ros_distro: jazzy\n"; + write_file(dir / "metadata.yaml", yaml); + } + // What the fault manager stores: every regular file under the bag directory. uint64_t directory_total() const { uint64_t total = 0; for (const auto & entry : std::filesystem::recursive_directory_iterator(bag_dir_)) { if (entry.is_regular_file()) { - total += static_cast(entry.file_size()); + total += entry.file_size(); } } return total; @@ -302,7 +326,7 @@ TEST_F(RosbagBagDirectoryTest, DescriptorSizeIsTheBytesTheDownloadServesNotTheBa // bag directory to its storage file, then take that file's size. const std::string served_path = BulkDataHandlers::resolve_rosbag_file_path(bag_dir_.string()); ASSERT_EQ(served_path, (bag_dir_ / "recording_0.db3").string()); - const auto served_bytes = static_cast(std::filesystem::file_size(served_path)); + const uint64_t served_bytes = std::filesystem::file_size(served_path); // Not vacuous: the directory holds metadata.yaml as well, so the stored figure // and the served figure are genuinely different numbers. @@ -330,12 +354,555 @@ TEST_F(RosbagBagDirectoryTest, ServedBytesIsUnknownRatherThanZeroWhenTheBagIsNot EXPECT_FALSE(handlers::detail::rosbag_served_bytes("").has_value()); EXPECT_FALSE(handlers::detail::rosbag_served_bytes((bag_dir_ / "no_such_bag").string()).has_value()); - // An empty bag directory resolves to no storage file at all. + // A directory with no metadata.yaml and no storage file in it. The metadata + // gate is what declines here, before the resolver is reached: the bag does not + // say how many storage files it holds, so this side will not guess one. The + // resolver would also find nothing here, which is not what this case turns on. const auto empty_bag = bag_dir_ / "empty_bag"; std::filesystem::create_directories(empty_bag); EXPECT_FALSE(handlers::detail::rosbag_served_bytes(empty_bag.string()).has_value()); } +TEST_F(RosbagBagDirectoryTest, ASplitRecordingIsListedAtTheRowsFigureNotAtOneSegment) { + // Past the configured maximum bag size rosbag2 splits a recording across + // several storage files. The download route hands over whichever one the + // resolver reaches first, so no single file is the transfer, and sizing the + // descriptor by that file advertised a split recording at the size of one part + // of it. The fault manager reports the recording's total for a split, and the + // two API surfaces have to agree on one recording. + const auto split_dir = bag_dir_ / "split"; + std::filesystem::create_directories(split_dir); + write_file(split_dir / "split_0.db3", std::string(16384, 'a')); + write_file(split_dir / "split_1.db3", std::string(53248, 'b')); + write_metadata(split_dir, {"split_0.db3", "split_1.db3"}); + + uint64_t split_total = 0; + for (const auto & entry : std::filesystem::recursive_directory_iterator(split_dir)) { + if (entry.is_regular_file()) { + split_total += entry.file_size(); + } + } + const uint64_t first_segment = std::filesystem::file_size(split_dir / "split_0.db3"); + const uint64_t second_segment = std::filesystem::file_size(split_dir / "split_1.db3"); + + // The row carries what the fault manager reports for a split: the total. + const json row{{"fault_code", "SPLIT_FAULT"}, + {"recording_id", "fault_SPLIT_FAULT_1738664999000"}, + {"file_path", split_dir.string()}, + {"format", "sqlite3"}, + {"duration_sec", 6.0}, + {"size_bytes", split_total}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, split_total) << "a split recording keeps the figure the fault manager reported"; + EXPECT_NE(descriptors[0].size, first_segment) << "one segment is not the recording"; + EXPECT_NE(descriptors[0].size, second_segment) << "and neither is the other"; + + // The helper declines, which is what makes the fallback fire. + EXPECT_FALSE(handlers::detail::rosbag_served_bytes(split_dir.string()).has_value()); +} + +TEST_F(RosbagBagDirectoryTest, ABareStorageFileIsItsOwnRecordingAndIsSizedAsSuch) { + // A row's file_path can be the storage file itself. The resolver accepts that + // shape and the download serves it, so the listing has to size it too. A bare + // file has no metadata.yaml beside it under that name, so the metadata gate sits + // behind the is_storage_file test: in front of it, every such row is declined + // here and listed at zero the moment the row carries no figure of its own. + const auto bare_file = bag_dir_ / "standalone_recording.db3"; + write_file(bare_file, std::string(7168, 'z')); + const uint64_t bare_size = std::filesystem::file_size(bare_file); + + EXPECT_EQ(handlers::detail::rosbag_served_bytes(bare_file.string()), bare_size); + + const json row{{"fault_code", "BARE_FILE_FAULT"}, + {"recording_id", "standalone_recording.db3"}, + {"file_path", bare_file.string()}, + {"format", "sqlite3"}, + {"size_bytes", 1}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, bare_size) << "a bare storage file is measured, not declined"; + EXPECT_NE(descriptors[0].size, 1u) << "and the row's figure is not what was reported"; +} + +TEST_F(RosbagBagDirectoryTest, TheMetadataNamesTheStorageFileRatherThanDirectoryOrder) { + // A stray .db3 beside the recording - a leftover segment, a copy - is servable + // and sizeable in place of the real one for anything that takes whichever file + // the directory iterator yields first. The fault manager sizes the single file + // its metadata names, a split falling back to the stored total + // (rosbag_capture.cpp, rosbag_served_bytes), so the two sides answer + // differently for the same directory unless the gateway reads that field too. + // The bag's own metadata is the tie-break on both sides. + const auto strays = bag_dir_ / "with_stray"; + std::filesystem::create_directories(strays); + write_file(strays / "recording_0.db3", std::string(4096, 'a')); + write_file(strays / "recording_1.db3", std::string(65536, 'b')); + // Every file the bag will hold exists before the order below is sampled. + write_metadata(strays, {"recording_0.db3"}); + + // Which storage file the resolver's fallback loop reaches first here, walked + // the way that loop walks. Directory order belongs to the filesystem, so the + // test samples it: metadata naming a fixed file can agree with the loop, and + // where it agrees the metadata could be ignored entirely and the test would + // still pass. Naming the OTHER file is the naming the loop cannot reproduce. + std::filesystem::path first_in_directory_order; + for (const auto & entry : std::filesystem::directory_iterator(strays)) { + if (!entry.is_regular_file()) { + continue; + } + const auto ext = entry.path().extension().string(); + if (ext == ".db3" || ext == ".mcap") { + first_in_directory_order = entry.path(); + break; + } + } + ASSERT_FALSE(first_in_directory_order.empty()) << "no storage file was walked, so nothing is being told apart"; + + const std::filesystem::path named = first_in_directory_order.filename() == "recording_0.db3" + ? strays / "recording_1.db3" + : strays / "recording_0.db3"; + write_metadata(strays, {named.filename().string()}); + + const uint64_t named_size = std::filesystem::file_size(named); + const uint64_t stray_size = std::filesystem::file_size(first_in_directory_order); + ASSERT_NE(named_size, stray_size) << "the two files are the same size, so nothing is being told apart"; + + // The download resolves the named file, so the bytes on the wire are its bytes. + EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(strays.string()), named.string()); + EXPECT_EQ(handlers::detail::rosbag_served_bytes(strays.string()), named_size); + + const json row{{"fault_code", "STRAY_FAULT"}, + {"recording_id", "with_stray"}, + {"file_path", strays.string()}, + {"format", "sqlite3"}, + {"size_bytes", 999999}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + // The fault manager reports the size of the single file its metadata names for + // this same shape, so both sides describe one recording with one number. Its + // behaviour is pinned by ASingleNamedFileIsSizedEvenWithAStrayFileBesideIt in + // test_rosbag_capture.cpp. + EXPECT_EQ(descriptors[0].size, named_size) << "the listing must report the file the bag names"; + EXPECT_NE(descriptors[0].size, stray_size) << "directory order must not decide which file a recording is"; +} + +TEST_F(RosbagBagDirectoryTest, ANamedStorageFileThatIsGoneIsNotMeasuredByWhateverIsLeftBesideIt) { + // The bag names one storage file and that file is not there - an eviction, an + // interrupted copy - while something else with a storage extension still is. + // The resolver falls through to directory order so the download has a file to + // hand over, but a size is a promise about the recording and a stray file is + // not the recording. The fault manager answers this shape with the stored total + // (ANamedFileThatIsNotOnDiskFallsBackToTheStoredTotal in test_rosbag_capture.cpp), + // and declining here is what leaves the descriptor carrying that same figure. + const auto ghost = bag_dir_ / "ghost"; + std::filesystem::create_directories(ghost); + write_file(ghost / "recording_1.db3", std::string(65536, 'b')); + write_metadata(ghost, {"recording_0.db3"}); // a segment that was never written + + // Not vacuous: the resolver does answer here, with the file the bag never named. + ASSERT_EQ(BulkDataHandlers::resolve_rosbag_file_path(ghost.string()), (ghost / "recording_1.db3").string()); + EXPECT_FALSE(handlers::detail::rosbag_served_bytes(ghost.string()).has_value()) + << "a file the bag never named was measured as the recording"; + + const json row{{"fault_code", "GHOST_FAULT"}, + {"recording_id", "ghost"}, + {"file_path", ghost.string()}, + {"format", "sqlite3"}, + {"size_bytes", 4242}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, 4242u) << "the descriptor must keep the figure the row carried"; + EXPECT_NE(descriptors[0].size, std::filesystem::file_size(ghost / "recording_1.db3")) + << "the stray file's size reached the listing"; +} + +namespace { + +/// What this side answers for a bag whose metadata names something it will not +/// follow: the resolver hands back nothing (the directory holds no storage file +/// of its own in these shapes), the size helper declines, and the descriptor +/// keeps the figure the row carried. @p must_not_be_served is the path the name +/// points at, named here so a failure says which one leaked. +void expect_bag_names_nothing_followable(const std::filesystem::path & bag, + const std::filesystem::path & must_not_be_served, uint64_t row_figure) { + const std::string resolved = BulkDataHandlers::resolve_rosbag_file_path(bag.string()); + EXPECT_EQ(resolved, "") << "the resolver followed the metadata to " << must_not_be_served; + EXPECT_FALSE(handlers::detail::rosbag_served_bytes(bag.string()).has_value()) + << "a file outside the bag was measured as the recording"; + + const json row{{"fault_code", "UNFOLLOWABLE_NAME"}, + {"recording_id", bag.filename().string()}, + {"file_path", bag.string()}, + {"format", "sqlite3"}, + {"size_bytes", row_figure}}; + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, row_figure) << "the descriptor must keep the figure the row carried"; +} + +} // namespace + +// The four shapes below are one rule seen from four sides: the name in +// metadata.yaml chooses a file the download hands over and advertises a length +// for, so it may only ever choose a direct child of the bag directory that +// carries a storage extension - which is all rosbag2 ever writes. Path +// concatenation does not enforce that on its own: an absolute name replaces the +// directory outright and a `..` name climbs out of it, so a bag directory a +// robot can write would otherwise be able to point the route at any file the +// gateway can read. +TEST_F(RosbagBagDirectoryTest, AMetadataNameThatClimbsOutOfTheBagIsNotFollowed) { + const auto escape_target = bag_dir_ / "escape.db3"; + write_file(escape_target, std::string(8192, 'e')); + + const auto bag = bag_dir_ / "escaping_bag"; + std::filesystem::create_directories(bag); + write_metadata(bag, {"../escape.db3"}); + + // Not vacuous: the file the name points at exists and would be servable. + ASSERT_TRUE(std::filesystem::is_regular_file(escape_target)); + expect_bag_names_nothing_followable(bag, escape_target, 4242); +} + +TEST_F(RosbagBagDirectoryTest, AnAbsoluteMetadataNameIsNotFollowed) { + const auto outside = bag_dir_ / "outside_the_bag.db3"; + write_file(outside, std::string(12288, 'o')); + + const auto bag = bag_dir_ / "absolute_bag"; + std::filesystem::create_directories(bag); + write_metadata(bag, {outside.string()}); + ASSERT_TRUE(std::filesystem::is_regular_file(outside)); + expect_bag_names_nothing_followable(bag, outside, 4242); + + // The same rule with a host file that is not a recording at all, which is where + // an absolute name points when it is chosen deliberately. + write_metadata(bag, {"/etc/hostname"}); + expect_bag_names_nothing_followable(bag, "/etc/hostname", 4242); +} + +TEST_F(RosbagBagDirectoryTest, AMetadataNameInASubdirectoryIsNotFollowed) { + const auto bag = bag_dir_ / "nested_bag"; + const auto sub = bag / "sub"; + std::filesystem::create_directories(sub); + write_file(sub / "recording_0.db3", std::string(16384, 'n')); + write_metadata(bag, {"sub/recording_0.db3"}); + + // The directory scan does not descend either, so the two agree about what is + // in the bag. + ASSERT_TRUE(std::filesystem::is_regular_file(sub / "recording_0.db3")); + expect_bag_names_nothing_followable(bag, sub / "recording_0.db3", 4242); +} + +TEST_F(RosbagBagDirectoryTest, AnUnfollowableNameStillLeavesTheDownloadTheFileTheBagHolds) { + // Where the resolver and the size helper part company. A name neither will + // follow sends the resolver to the directory scan, which finds the recording + // sitting in the bag, so the download has a file to hand over. The size is a + // promise about the recording the bag claims, and the bag claims a file outside + // itself, so the listing declines and the descriptor keeps the row's figure. + const auto outside = bag_dir_ / "outside_the_bag.db3"; + write_file(outside, std::string(8192, 'o')); + + const auto bag = bag_dir_ / "bag_with_its_own_file"; + std::filesystem::create_directories(bag); + write_file(bag / "recording_0.db3", std::string(3072, 'r')); + write_metadata(bag, {"../outside_the_bag.db3"}); + + EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(bag.string()), (bag / "recording_0.db3").string()) + << "the download must be handed the storage file the bag directory holds"; + EXPECT_FALSE(handlers::detail::rosbag_served_bytes(bag.string()).has_value()) + << "a bag naming a file outside itself was given a size"; + + const json row{{"fault_code", "OUTSIDE_NAME_FAULT"}, + {"recording_id", "bag_with_its_own_file"}, + {"file_path", bag.string()}, + {"format", "sqlite3"}, + {"size_bytes", 4242}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].size, 4242u) << "the descriptor must keep the figure the row carried"; + EXPECT_NE(descriptors[0].size, std::filesystem::file_size(outside)) << "the file outside the bag reached the listing"; + EXPECT_NE(descriptors[0].size, std::filesystem::file_size(bag / "recording_0.db3")) + << "the served file was advertised for a recording the bag does not claim"; +} + +TEST_F(RosbagBagDirectoryTest, AMetadataNameWithoutAStorageExtensionIsNotFollowed) { + const auto bag = bag_dir_ / "text_bag"; + std::filesystem::create_directories(bag); + write_file(bag / "recording_0.txt", std::string(2048, 't')); + write_metadata(bag, {"recording_0.txt"}); + + // The directory scan accepts .db3 and .mcap only; the named file goes through + // that same gate. + ASSERT_TRUE(std::filesystem::is_regular_file(bag / "recording_0.txt")); + expect_bag_names_nothing_followable(bag, bag / "recording_0.txt", 4242); +} + +// A bag directory this process cannot walk must cost its own row and nothing +// else. The resolver used the throwing filesystem overloads, so one EACCES or +// ENOENT threw out of the listing handler, which has no catch in its chain, and +// the whole request answered 500 - every recording of the entity gone because of +// one unreadable directory. +// +// The trigger here is a symlink loop, whose ELOOP is refused whatever the caller +// holds, and these tests do not run under one identity: in CI they run as root, +// the workflow's jobs declaring a plain `container:` with no `user:` key, and +// locally as uid 1000. Root bypasses mode bits, so the mode-0000 shape only +// denies anything once the capabilities that do the bypassing are given up, which +// is what the second test below does; this one needs no such setup. +class UnreadableBagTest : public ::testing::Test { + protected: + void SetUp() override { + root_ = std::filesystem::temp_directory_path() / + ("bulkdata_unreadable_" + std::to_string(getpid()) + "_" + std::to_string(counter_++)); + std::filesystem::create_directories(root_); + + // Readable control bag: one storage file, real metadata. + readable_ = root_ / "readable_bag"; + std::filesystem::create_directories(readable_); + { + std::ofstream out(readable_ / "recording_0.db3", std::ios::binary); + out << std::string(2048, 'x'); + } + { + std::ofstream out(readable_ / "metadata.yaml", std::ios::binary); + out << "rosbag2_bagfile_information:\n version: 9\n relative_file_paths:\n - recording_0.db3\n"; + } + + // Unreadable bag: a symlink pointing at itself. Every filesystem query on it + // fails with ELOOP, for root as much as for anyone else. + loop_ = root_ / "loop_bag"; + std::error_code ec; + std::filesystem::create_symlink(loop_, loop_, ec); + symlink_created_ = !ec; + } + + void TearDown() override { + std::error_code ec; + std::filesystem::remove_all(root_, ec); + } + + std::filesystem::path root_; + std::filesystem::path readable_; + std::filesystem::path loop_; + bool symlink_created_{false}; + static int counter_; +}; + +int UnreadableBagTest::counter_ = 0; + +TEST_F(UnreadableBagTest, AnUnreadableBagCostsItsOwnRowAndNotTheListing) { + ASSERT_TRUE(symlink_created_) << "could not create the symlink loop, so nothing is being tested"; + // The loop really is refused by the filesystem, whatever uid this runs as. + std::error_code probe_ec; + const bool loop_is_a_directory = std::filesystem::is_directory(loop_, probe_ec); + ASSERT_TRUE(static_cast(probe_ec)) << "the symlink loop resolved, so it is not an unreadable bag"; + ASSERT_FALSE(loop_is_a_directory) << "a path that errored cannot also be a readable directory"; + + const uint64_t readable_served = std::filesystem::file_size(readable_ / "recording_0.db3"); + + const std::vector rows{ + json{{"fault_code", "READABLE"}, + {"recording_id", "fault_READABLE_1"}, + {"file_path", readable_.string()}, + {"format", "sqlite3"}, + {"size_bytes", 999999}}, + json{{"fault_code", "UNREADABLE"}, + {"recording_id", "fault_UNREADABLE_1"}, + {"file_path", loop_.string()}, + {"format", "sqlite3"}, + {"size_bytes", 4242}}, + }; + + // The listing answers, and answers with BOTH recordings. + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 2u) << "an unreadable bag removed another recording from the listing"; + EXPECT_EQ(descriptors[0].id, "fault_READABLE_1"); + EXPECT_EQ(descriptors[0].size, readable_served) << "the readable bag is still measured locally"; + EXPECT_EQ(descriptors[1].id, "fault_UNREADABLE_1"); + EXPECT_EQ(descriptors[1].size, 4242u) << "the unreadable bag keeps the figure its row carried"; + + // These two are the assertions that pin the throwing overloads; the listing + // assertion above is defence in depth. Order is why: rosbag_served_bytes reads + // the bag's metadata before it resolves any file, and an unreadable bag has no + // readable metadata either, so it returns nullopt before the resolver is ever + // reached. download() has no such gate in front of + // it - it calls the resolver directly - so the resolver's own refusal to throw + // is what that route depends on, and it is asserted here directly. + EXPECT_NO_THROW({ EXPECT_FALSE(handlers::detail::rosbag_served_bytes(loop_.string()).has_value()); }); + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(loop_.string()), ""); }); +} + +namespace { + +/// Everything a bag directory the caller cannot read has to survive: the size +/// helper, the resolver the download route calls directly, and the fold that +/// builds the listing from a row pointing at it. +/// +/// Returns the first reason a check did not hold, empty when all of them did. +/// The reason is what the caller reports, because not every way this body can +/// fail is a defect in the code under test: an ancestor of the bag the caller +/// cannot traverse denies everything here too, and that has to read as the +/// environment it is. +std::string locked_bag_declined_reason(const std::filesystem::path & locked, uint64_t stored_figure) { + std::string reason; + const auto check = [&reason](bool condition, const std::string & what) { + if (!condition && reason.empty()) { + reason = what; + } + }; + + try { + // The bag itself is reachable, so what denies below is the bag's own mode. + std::error_code reach_ec; + check(std::filesystem::is_directory(locked, reach_ec) && !reach_ec, + "an ancestor of " + locked.string() + " is not traversable by this caller, so the path to the bag " + + "denied before the bag's own mode could"); + + // The permission is really denied, so nothing below can pass on a directory + // that was readable all along. + std::error_code probe_ec; + std::filesystem::directory_iterator probe(locked, probe_ec); + check(static_cast(probe_ec), "the directory walked, so its mode denied nothing to this caller"); + + check(!handlers::detail::rosbag_served_bytes(locked.string()).has_value(), + "an unreadable bag was given a size of its own"); + check(BulkDataHandlers::resolve_rosbag_file_path(locked.string()).empty(), + "an unreadable bag resolved to a storage file"); + + const json row{{"fault_code", "LOCKED"}, + {"recording_id", "fault_LOCKED_1"}, + {"file_path", locked.string()}, + {"format", "sqlite3"}, + {"size_bytes", stored_figure}}; + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + check(descriptors.size() == 1u, "the listing lost the row of an unreadable bag"); + if (descriptors.size() == 1u) { + check(descriptors[0].size == stored_figure, "the unreadable bag did not keep the figure its row carried"); + } + } catch (const std::exception & e) { + check(false, std::string("a filesystem call threw: ") + e.what()); + } + return reason; +} + +/// Puts back what the locked-bag test takes away: the bag directory's mode +/// always, and the calling thread's effective capability word once +/// `arm_capabilities` has recorded one. A destructor, so an assertion that +/// returns early or an exception on any path still leaves the fixture a +/// directory it can remove and the thread the capabilities the process started +/// with - the rest of the suite runs on that thread. +class LockedBagRestore { + public: + explicit LockedBagRestore(std::filesystem::path locked) : locked_(std::move(locked)) { + } + + LockedBagRestore(const LockedBagRestore &) = delete; + LockedBagRestore & operator=(const LockedBagRestore &) = delete; + LockedBagRestore(LockedBagRestore &&) = delete; + LockedBagRestore & operator=(LockedBagRestore &&) = delete; + + ~LockedBagRestore() { + if (capabilities_armed_) { + __user_cap_header_struct header{}; + header.version = _LINUX_CAPABILITY_VERSION_3; + header.pid = 0; // the calling thread + __user_cap_data_struct capabilities[2]{}; + bool restored = ::syscall(SYS_capget, &header, capabilities) == 0; + if (restored) { + capabilities[0].effective = saved_effective_; + restored = ::syscall(SYS_capset, &header, capabilities) == 0; + } + if (!restored) { + ADD_FAILURE() << "this thread's effective capabilities could not be put back: " << std::strerror(errno); + } + } + std::error_code ec; + std::filesystem::permissions(locked_, std::filesystem::perms::owner_all, ec); + } + + void arm_capabilities(__u32 saved_effective) { + saved_effective_ = saved_effective; + capabilities_armed_ = true; + } + + private: + std::filesystem::path locked_; + __u32 saved_effective_{0}; + bool capabilities_armed_{false}; +}; + +} // namespace + +// The second shape: a directory whose mode denies everyone. Mode bits decide +// nothing for a caller holding CAP_DAC_OVERRIDE or CAP_DAC_READ_SEARCH, which +// root holds (in the CI container CAP_DAC_OVERRIDE; CAP_DAC_READ_SEARCH is not +// in Docker's default set, and clearing it is a no-op there), and this test runs +// as root in CI - the workflow's jobs declare a plain `container:` with no +// `user:` key - and as an ordinary uid locally. So it takes those two out of its +// own thread's effective set for the length of the body. Capabilities are +// per-thread and giving one up is always permitted, so this needs no second +// process: the body runs on the test's own thread, where sanitizers still check +// it and coverage still counts it. +TEST_F(UnreadableBagTest, AModeZeroDirectoryIsAlsoDeclinedRatherThanThrown) { + // Every component above the locked bag stays traversable, so what stops the + // body is the bag's own mode and not the path leading to it. + std::error_code ec; + std::filesystem::permissions( + root_, + std::filesystem::perms::owner_all | std::filesystem::perms::group_exec | std::filesystem::perms::others_exec, ec); + ASSERT_FALSE(static_cast(ec)) << "could not make the temp root traversable"; + + const auto locked = root_ / "locked_bag"; + std::filesystem::create_directories(locked); + { + std::ofstream out(locked / "recording_0.db3", std::ios::binary); + out << std::string(1024, 'x'); + } + // A complete bag, metadata included, so every answer below turns on the + // permission. + { + std::ofstream out(locked / "metadata.yaml", std::ios::binary); + out << "rosbag2_bagfile_information:\n version: 9\n relative_file_paths:\n - recording_0.db3\n"; + } + std::filesystem::permissions(locked, std::filesystem::perms::none, ec); + ASSERT_FALSE(static_cast(ec)) << "could not drop the directory's permissions"; + + constexpr uint64_t kStoredFigure = 4242; + std::string reason; + std::string capability_error; + LockedBagRestore restore(locked); + + if (::geteuid() == 0) { + __user_cap_header_struct header{}; + header.version = _LINUX_CAPABILITY_VERSION_3; + header.pid = 0; // the calling thread + __user_cap_data_struct capabilities[2]{}; + if (::syscall(SYS_capget, &header, capabilities) != 0) { + capability_error = std::string("capget: ") + std::strerror(errno); + } else { + // Effective only. The permitted set keeps both, which is what lets the + // guard put them back. + restore.arm_capabilities(capabilities[0].effective); + capabilities[0].effective &= ~((1U << CAP_DAC_OVERRIDE) | (1U << CAP_DAC_READ_SEARCH)); + if (::syscall(SYS_capset, &header, capabilities) != 0) { + capability_error = std::string("capset while dropping: ") + std::strerror(errno); + } else { + reason = locked_bag_declined_reason(locked, kStoredFigure); + } + } + } else { + reason = locked_bag_declined_reason(locked, kStoredFigure); + } + + ASSERT_TRUE(capability_error.empty()) << "this thread's capabilities could not be put where the test needs them: " + << capability_error; + EXPECT_TRUE(reason.empty()) << reason; +} + TEST_F(RosbagBagDirectoryTest, AnUnreachableBagKeepsTheStoredFigureRatherThanReportingZero) { const json row{{"fault_code", "MOTOR_OVERHEAT"}, {"recording_id", "fault_MOTOR_OVERHEAT_1738664999000"}, @@ -478,8 +1045,8 @@ TEST_F(BulkDataHandlersTest, PayloadTooLargeErrorCodeDefined) { // // Pin the entity-type branching that drives rosbag descriptor lookups + the // download ownership check. Crucial because synthetic / runtime-discovered -// components have empty fqn AND empty namespace_path: without aggregation -// from hosted apps the handler used to silently return zero source filters. +// components have empty fqn AND empty namespace_path, so the filters come from +// aggregation over the apps they host; without it the handler has none to give. // // Tested as a pure free function in detail:: against a directly-constructed // ThreadSafeEntityCache so no GatewayNode / DDS context is needed. @@ -802,7 +1369,7 @@ TEST_F(BulkDataSourceFiltersTest, ARecordingIdUrlIsNotTheCompatibilityPath) { TEST_F(BulkDataSourceFiltersTest, APeerWithoutRecordingIdsIsNotTreatedAsCompatibilityPath) { // An older peer answers by fault code only and sends no recording id. The // attached-codes fallback already reduces to the pre-#620 check there, so - // demanding a second one would 404 downloads that used to work. + // demanding a second one would 404 a download such a peer can serve. const nlohmann::json older_peer = {{"file_path", "/var/bags/fault_MOTOR_1"}}; EXPECT_FALSE(handlers::detail::rosbag_resolved_by_fault_code(older_peer, "MOTOR_OVERHEAT")); } diff --git a/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py index 0c70027f5..d2faf125e 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py @@ -219,6 +219,51 @@ def test_bulk_data_unknown_category_returns_404(self): data = response.json() self.assertIn('error_code', data) + def test_bulk_data_descriptor_size_is_the_download_length(self): + """The descriptor's size is the number of bytes the download sends. + + A client sizes a buffer or a progress bar from the listing, so the + listing has to promise what the transfer delivers. Nothing else here + connects the two: the structure test above only checks that a size + field exists, and a wrong number passes that. + + The test recording is far below snapshots.rosbag.max_bag_size_mb, so it + is held in one storage file. That is the case where the descriptor size + and the download length are defined to be equal; a split recording is + reported at its total and is deliberately larger than its download. + + @verifies REQ_INTEROP_073 + """ + data = self.poll_endpoint_until( + '/apps/lidar_sensor/bulk-data/rosbags', + lambda d: d if d.get('items') else None, + timeout=10.0, + interval=1.0, + ) + self.assertGreater( + len(data['items']), 0, 'Expected at least one rosbag descriptor', + ) + descriptor = data['items'][0] + + response = requests.get( + f'{self.BASE_URL}/apps/lidar_sensor/bulk-data/rosbags/' + f'{descriptor["id"]}', + timeout=30, + ) + self.assertEqual(response.status_code, 200) + + body = response.content + self.assertGreater(len(body), 0, 'Download served an empty body') + self.assertEqual( + descriptor['size'], len(body), + f'Listing promised {descriptor["size"]} bytes and the download ' + f'sent {len(body)}', + ) + self.assertEqual( + int(response.headers['Content-Length']), len(body), + 'Content-Length disagrees with the body it described', + ) + def test_bulk_data_download_not_found(self): """Bulk-data download returns 404 for invalid UUID. From 5713977b4629629b319282af9a5d8d1fe6b630a3 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 14 Sep 2026 18:12:23 +0200 Subject: [PATCH 5/6] fix(fault_manager): follow a bag's metadata only to a storage file inside it rosbag_served_bytes sizes the named file only when the name is a direct child of the bag directory with a .db3 or .mcap extension; any other name falls back to the stored total, the same rule the gateway applies. The stray-file test's cross-reference names that rule. --- .../rosbag_capture.hpp | 10 +- .../src/rosbag_capture.cpp | 51 ++++++++-- .../test/test_fault_manager.cpp | 6 +- .../test/test_rosbag_capture.cpp | 93 ++++++++++++++++++- 4 files changed, 140 insertions(+), 20 deletions(-) diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp index 3e85ea31c..5189a00db 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp @@ -499,7 +499,7 @@ class RosbagCapture { /// /// The file is the one ``metadata.yaml`` names in ``relative_file_paths``, read through /// the same library that wrote it, so this answers with the bag's own record of its -/// contents rather than by guessing from a file extension. +/// contents. /// /// Falls back to @p stored_total_bytes, never to zero, when no single served file can /// be named: @@ -507,12 +507,16 @@ class RosbagCapture { /// - ``relative_file_paths`` naming other than exactly one file. Past /// ``max_bag_size_mb`` rosbag2 splits a recording across several storage files, and /// then no single number describes the download at all. +/// - a name that points outside the bag directory, or at something other than a +/// ``.db3`` or ``.mcap`` file. A name decides which file on the host is measured, +/// so only a storage file that is a direct child of the bag is followed. The +/// gateway applies the same rule to the same field. /// - a named file that cannot be stat'd. /// /// None of those is an error worth logging. A pre-metadata bag and a split bag are /// both normal, this runs once per reported row on every request, and the fallback is -/// a real measurement of the recording rather than a failure sentinel. A zero would -/// not be: it would describe the recording as empty. +/// a real measurement of the recording. A zero would describe the recording as +/// empty. /// /// @param bag_path Bag directory as stored in ``RosbagFileInfo::file_path`` /// @param stored_total_bytes The stored directory total, used as the fallback diff --git a/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp b/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp index 40afd6238..b755b1f33 100644 --- a/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp @@ -1265,6 +1265,37 @@ std::string fnv1a_hex(const std::string & text) { return oss.str(); } +/// The file @p relative_name names inside the bag at @p bag_path, when that is a +/// name this side follows: a direct child of the bag directory carrying a storage +/// extension, which is what rosbag2 writes. +/// +/// Path concatenation carries no such guarantee. An absolute name replaces the bag +/// directory outright and a `..` name climbs out of it, so without this test a +/// metadata.yaml chooses which file on the host gets measured and reported as the +/// recording's size. The gateway applies the same rule in its own +/// rosbag_named_storage_file (bulkdata_handlers.cpp); the two packages share no +/// code, so the rule is written out on both sides and a change to it belongs on +/// both. +/// +/// nullopt when the name is not one of those. The caller treats that as it treats +/// a named file that is absent: the stored total. +std::optional named_storage_file(const std::string & bag_path, + const std::string & relative_name) { + std::filesystem::path bag = std::filesystem::path(bag_path).lexically_normal(); + if (!bag.has_filename()) { + bag = bag.parent_path(); // tolerate a trailing slash + } + const std::filesystem::path named = (bag / relative_name).lexically_normal(); + if (named.parent_path() != bag) { + return std::nullopt; + } + const std::string extension = named.extension().string(); + if (extension != ".db3" && extension != ".mcap") { + return std::nullopt; + } + return named; +} + } // namespace std::string RosbagCapture::bag_directory_name(const std::string & fault_code, int64_t timestamp_ms) { @@ -1356,14 +1387,13 @@ size_t RosbagCapture::calculate_bag_size(const std::string & bag_path) const { // The bytes a download of this recording actually transfers, which is the one // storage file the bulk-data route hands over. calculate_bag_size() above answers // the storage question (what the recording costs on disk) and this one answers the -// client's question (what is about to arrive). Reporting the footprint in place of -// the transfer is what made every listing overstate its own download by -// metadata.yaml. Keeping them separate is what lets the quota stay honest while the -// API does. +// client's question (what is about to arrive). The two are separate figures, which +// is what lets the quota and the API each stay honest: a footprint reported here +// would overstate every download by metadata.yaml. // -// The served file is read out of the bag's own metadata.yaml rather than guessed -// from a file extension, so a bag that names something unexpected is still described -// by its own record. See the header for every fallback and why none of them logs. +// The served file is the one the bag's own metadata.yaml names, and only when that +// name points at a storage file inside the bag directory. See the header for every +// fallback and why none of them logs. size_t rosbag_served_bytes(const std::string & bag_path, size_t stored_total_bytes) { try { rosbag2_storage::MetadataIo metadata_io; @@ -1380,9 +1410,12 @@ size_t rosbag_served_bytes(const std::string & bag_path, size_t stored_total_byt return stored_total_bytes; } - const std::filesystem::path storage_file = std::filesystem::path(bag_path) / metadata.relative_file_paths.front(); + const auto storage_file = named_storage_file(bag_path, metadata.relative_file_paths.front()); + if (!storage_file) { + return stored_total_bytes; + } std::error_code ec; - const auto served = std::filesystem::file_size(storage_file, ec); + const auto served = std::filesystem::file_size(*storage_file, ec); if (ec) { return stored_total_bytes; } diff --git a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp index d92c53c94..33b0d4311 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -1657,8 +1657,8 @@ TEST_F(FaultEventPublishingTest, GetFaultReportsARecordingsServedBytesNotItsFoot } // The other three services that quote a recording's size. GetFault above covers the -// snapshot entry. These are the remaining answers, each reached through its own -// service call rather than through the helper. +// snapshot entry. These are the remaining answers, each driven through its own +// service call, so what every assertion here reads is the service's own reply. TEST_F(FaultEventPublishingTest, EveryRosbagServiceReportsTheServedBytes) { ReportedBag bag("all_services"); ASSERT_GT(bag.footprint, bag.served) << "metadata.yaml did not land, so there is nothing to tell apart"; @@ -1721,7 +1721,7 @@ TEST_F(FaultEventPublishingTest, EveryRosbagServiceReportsTheServedBytes) { // The legacy /faults/{code}/snapshots/bag route was removed, so a payload naming it // hands the caller a 404. Nothing in this repo reads the field, and a recording is // addressed under its entity, which the fault manager cannot resolve, so the field -// is gone rather than repointed. +// is gone. TEST_F(FaultEventPublishingTest, GetSnapshotsDoesNotAdvertiseARouteThatWasRemoved) { ReportedBag bag("no_download_url"); diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp index c8c75f149..3050a6be6 100644 --- a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp @@ -578,7 +578,7 @@ namespace { /// A bag directory carrying a real ``metadata.yaml``, written by the same library /// rosbag2 writes it with, so the parse under test is the parse that runs in -/// production rather than a hand-copied literal that can drift from it. +/// production. class ServedBytesBag { public: explicit ServedBytesBag(const std::string & label) { @@ -700,15 +700,98 @@ TEST(RosbagServedBytesTest, ANamedFileThatIsNotOnDiskFallsBackToTheStoredTotal) EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), stored_total); } +namespace { + +/// A name the size helper follows only when it lands on a storage file inside the +/// bag directory. Every case below is checked twice on one bag: first with an +/// in-bag name, which must answer with that file, and then with @p +/// unfollowable_name, which must answer with the stored total. The control is what +/// separates the rule from a metadata document rosbag2 wrote but cannot read back - +/// both would fall back, for entirely different reasons. +void expect_unfollowable_name_falls_back(ServedBytesBag & bag, const std::string & unfollowable_name, + const std::string & in_bag_name) { + bag.write_metadata({in_bag_name}); + ASSERT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), bag.directory_total()), + bag.file_size_of(in_bag_name)) + << "the in-bag control name was not followed, so this bag says nothing about " << unfollowable_name; + + bag.write_metadata({unfollowable_name}); + const size_t stored_total = bag.directory_total(); + EXPECT_EQ(ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total), stored_total) + << "a name pointing outside the bag was measured as the recording: " << unfollowable_name; +} + +} // namespace + +// A name in metadata.yaml decides which file on the host this helper measures and +// reports as the recording's size, so it is followed only when it lands on a +// storage file that is a direct child of the bag directory. Path concatenation +// enforces none of that: an absolute name replaces the bag directory and a `..` +// name climbs out of it. +TEST(RosbagServedBytesTest, ANameThatClimbsOutOfTheBagFallsBackToTheStoredTotal) { + ServedBytesBag bag("escape"); + const std::string in_bag = bag.add_storage_file("recording_0.db3", 4096); + + const std::string outside_name = "served_bytes_outside_" + std::to_string(::getpid()) + ".db3"; + const auto outside = bag.dir().parent_path() / outside_name; + { + std::ofstream out(outside, std::ios::binary); + out << std::string(8192, 'o'); + } + ASSERT_TRUE(std::filesystem::is_regular_file(outside)) << "the file the name points at was not created"; + + expect_unfollowable_name_falls_back(bag, "../" + outside_name, in_bag); + + std::error_code ec; + std::filesystem::remove(outside, ec); +} + +TEST(RosbagServedBytesTest, AnAbsoluteNameFallsBackToTheStoredTotal) { + ServedBytesBag bag("absolute"); + const std::string in_bag = bag.add_storage_file("recording_0.db3", 4096); + + ServedBytesBag elsewhere("absolute_target"); + elsewhere.add_storage_file("recording_0.db3", 8192); + const std::string absolute_name = (elsewhere.dir() / "recording_0.db3").string(); + ASSERT_TRUE(std::filesystem::is_regular_file(absolute_name)); + expect_unfollowable_name_falls_back(bag, absolute_name, in_bag); + + // A host file that is not a recording at all, which is what an absolute name + // reaches for when this is an attack and not a stale bag. + expect_unfollowable_name_falls_back(bag, "/etc/hostname", in_bag); +} + +TEST(RosbagServedBytesTest, ANameInASubdirectoryFallsBackToTheStoredTotal) { + ServedBytesBag bag("nested"); + const std::string in_bag = bag.add_storage_file("recording_0.db3", 4096); + + std::filesystem::create_directories(bag.dir() / "sub"); + { + std::ofstream out(bag.dir() / "sub" / "segment.db3", std::ios::binary); + out << std::string(8192, 's'); + } + ASSERT_TRUE(std::filesystem::is_regular_file(bag.dir() / "sub" / "segment.db3")); + expect_unfollowable_name_falls_back(bag, "sub/segment.db3", in_bag); +} + +TEST(RosbagServedBytesTest, ANameWithoutAStorageExtensionFallsBackToTheStoredTotal) { + ServedBytesBag bag("text"); + const std::string in_bag = bag.add_storage_file("recording_0.db3", 4096); + bag.add_storage_file("recording_0.txt", 8192); + ASSERT_TRUE(std::filesystem::is_regular_file(bag.dir() / "recording_0.txt")); + expect_unfollowable_name_falls_back(bag, "recording_0.txt", in_bag); +} + TEST(RosbagServedBytesTest, ASingleNamedFileIsSizedEvenWithAStrayFileBesideIt) { // A leftover segment or a copy sitting beside the recording must not change // which file is measured. The metadata names one file and that is the file. // // This is also the fault manager's half of a cross-package agreement: the // gateway sizes the same directory shape through its own resolver, and its - // TheMetadataNamesTheStorageFileRatherThanDirectoryOrder asserts the same - // 4096. Directory order decided the gateway's answer until it read this field - // too, and the two sides reported different sizes for one recording. + // TheMetadataNamesTheStorageFileRatherThanDirectoryOrder pins the same rule: + // the size follows the file the metadata names, whichever file the directory + // yields first. Directory order decided the gateway's answer until it read + // this field too, and the two sides reported different sizes for one recording. ServedBytesBag bag("stray"); const std::string named = bag.add_storage_file("recording_0.db3", 4096); bag.add_storage_file("recording_1.db3", 65536); @@ -717,7 +800,7 @@ TEST(RosbagServedBytesTest, ASingleNamedFileIsSizedEvenWithAStrayFileBesideIt) { const size_t reported = ros2_medkit_fault_manager::rosbag_served_bytes(bag.path(), stored_total); EXPECT_EQ(reported, bag.file_size_of(named)); - EXPECT_EQ(reported, 4096u) << "the same number the gateway's listing reports for this shape"; + EXPECT_EQ(reported, 4096u) << "the rule the gateway's listing follows for this shape: the file the bag names"; EXPECT_NE(reported, bag.file_size_of("recording_1.db3")) << "the stray file is not the recording"; EXPECT_NE(reported, stored_total); } From cccf127d710295af570bd40f9451c7934b8a2773 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 14 Sep 2026 18:12:24 +0200 Subject: [PATCH 6/6] docs(api): state when a recording's three sizes agree and when they part rest.rst now gives the full condition. The descriptor size, size_bytes and Content-Length are one number when metadata.yaml names exactly one storage file, the name is a direct child of the bag with a .db3 or .mcap extension, and that file is present. The page lists the four cases where the reported size is the stored total while the download hands over one file. It states that a bag directory with no storage file answers 500. It also corrects the on-disk extension of a sqlite3 recording from .sqlite3 to .db3. --- docs/api/rest.rst | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 47e308823..977929d56 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1775,7 +1775,7 @@ is the time that recording was made. ``size`` is the number of bytes the download route below puts on the wire for that descriptor, so a client can size a buffer or a progress bar from the listing. For a rosbag held in a single storage file, which is the normal case, -that is the file (``.mcap`` or ``.sqlite3``) and it is the only file the +that is the file (``.mcap`` or ``.db3``) and it is the only file the download serves. The bag directory also holds ``metadata.yaml``, and those bytes are not part of the transfer. @@ -1783,21 +1783,33 @@ are not part of the transfer. **One recording, one size.** The descriptor ``size`` here, the ``environment_data.snapshots[].size_bytes`` a fault reports for the same -recording, and the ``Content-Length`` of its download are the same number, and -that number is the storage file. A recording also has a footprint on the -gateway host, which is larger because the directory holds ``metadata.yaml`` as -well. That figure is what the recording spends against its storage quota and is -not reported by the API. The one case where the two coincide is a recording -split across several storage files, past the configured maximum bag size: the -download can hand over only one of them, no single file describes the transfer, -and the API reports the recording's total instead. - -For that split case the three numbers stop agreeing, and deliberately so. The -descriptor ``size`` and the nested ``size_bytes`` report the recording's total -while the download's ``Content-Length`` is the one storage file it hands over, -so ``size`` exceeds ``Content-Length``. That gap is the signal: a client that -compares the two can tell the transfer it just made is a part of the recording -rather than the whole of it, which no single reported number could express. +recording, and the ``Content-Length`` of its download are the same number when +the bag's ``metadata.yaml`` names exactly one storage file, that name is a direct +child of the bag directory ending in ``.db3`` or ``.mcap``, and the file is +present - the normal case - and that number is the storage file. A recording also +has a footprint on the gateway host, which is larger because the directory holds +``metadata.yaml`` as well. That figure is what the recording spends against its +storage quota and is not reported by the API. + +Four shapes fall outside that condition, and there the three numbers part +deliberately: + +- a recording split across several storage files, past the configured maximum + bag size; +- a bag with no ``metadata.yaml``, or one that cannot be parsed; +- a bag whose named storage file is absent from the directory; +- a name that points outside the bag directory, or at a file that is neither + ``.db3`` nor ``.mcap``. The gateway and the fault manager both decline such a + name, because a name decides which file on the host is served and measured. + +In each of them the descriptor ``size`` and the nested ``size_bytes`` carry the +recording's stored total, while the download hands over whichever single storage +file the bag directory holds. ``size`` can therefore exceed ``Content-Length``, +and that gap is the signal in each of these cases: a client comparing the two can +tell the transfer it just made is a part of the recording and not the whole of +it, which no single reported number could express. When the directory holds no +storage file at all, the download has nothing to send and answers ``500``, so +there is no ``Content-Length`` to compare. Download Bulk Data ~~~~~~~~~~~~~~~~~~