From 8688c94a694850de73754a8ccb275865ef0cac28 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 8 Sep 2026 22:01:41 +0200 Subject: [PATCH 1/9] fix(gateway): size a rosbag descriptor by the bytes the download serves A rosbag2 recording is a directory holding one storage file plus metadata.yaml. The download route resolves the storage file and streams that alone, but the listing reported the figure the fault manager stores, which is the whole directory. The two numbers describe different things: the stored one is the recording's footprint against its disk quota, the listed one is what a client is about to fetch. Every listing therefore overstated the download, on a short recording by around a tenth of the transfer, and a client sizing a buffer or a progress bar from it never reached the end. The listing now measures the file the download resolves, through the same resolver, so the promised length is the length that arrives. A recording whose bag this process cannot see keeps the stored figure: it is the only number left, and a zero would describe the recording as empty rather than as unmeasured. The stored figure itself is untouched, so quota accounting still counts the bytes the recording occupies. Also drop the removed snapshot endpoints from the gateway README and the snapshots tutorial quick start. GET /faults/{code}/snapshots and .../snapshots/bag answer 404. Snapshots are returned inline with the fault, and recordings are downloaded through the bulk-data endpoints. The tutorial's migration table, which is what points a reader at the replacements, stays. --- 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 22bce7c629c125b972b9e9d9699d529d867c8cb6 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 12:22:52 +0200 Subject: [PATCH 2/9] fix(fault-manager): report a recording's served bytes, not its footprint 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, which is what the recording costs against max_total_storage_mb. Every API answer that quoted it overstated the download by metadata.yaml, and on a short recording that is around a tenth of the transfer. The environment_data.snapshots[] entry beside a download link was the worst placed of them, since that is the number a client sizes its transfer from. rosbag_served_bytes() now answers the reporting question separately: the storage file the bag's own metadata.yaml names, read through the library that wrote it rather than guessed from a file extension. The four service answers that quote a size go through it. RosbagFileInfo::size_bytes and the quota are untouched and still count the whole directory, which is what eviction frees. It falls back to the stored total, never to zero, when no single served file can be named: no metadata.yaml, one that cannot be parsed, a named file that is gone, or a recording split across several storage files past the maximum bag size. None of those is an error. The fallback is a real measurement of the recording, while a zero would describe it as empty. rest.rst now states the rule once: the descriptor size, the nested size_bytes and the download's Content-Length are the same number, and that number is the storage file. --- 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 06f9631077b568139cd66821228b92af68f0904c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 12:40:05 +0200 Subject: [PATCH 3/9] 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 has not existed since 0.2.0, so the field handed every caller a URL that answers 404 while looking like the way to fetch the recording. Nothing reads it. The field appears once in this repository, at the line that writes it, and the gateway builds its own entity-scoped bulk-data URI from the recording id it receives on the GetFault snapshot entries. It is dropped rather than repointed. A recording is addressed under its entity, as /api/v1/{entity-type}/{id}/bulk-data/rosbags/{recording_id}, and 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 not that mapping, so any URL built here would be a guess at one of four prefixes, and a plausible wrong URL is worse than no URL. The size the other three rosbag services report is now covered at the service level as well, alongside the GetFault snapshot entry that already was, and the shared bag fixture behind those tests is factored out. --- src/ros2_medkit_fault_manager/CHANGELOG.rst | 5 + .../src/fault_manager_node.cpp | 13 +- .../test/test_fault_manager.cpp | 212 ++++++++++++++---- 3 files changed, 187 insertions(+), 43 deletions(-) diff --git a/src/ros2_medkit_fault_manager/CHANGELOG.rst b/src/ros2_medkit_fault_manager/CHANGELOG.rst index 078574676..3bce45e15 100644 --- a/src/ros2_medkit_fault_manager/CHANGELOG.rst +++ b/src/ros2_medkit_fault_manager/CHANGELOG.rst @@ -2,6 +2,11 @@ Changelog for package ros2_medkit_fault_manager ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* **Breaking (service payload):** the ``GetSnapshots`` response no longer carries ``rosbag.download_url``. It named ``/api/v1/faults/{code}/snapshots/bag``, a route that has not existed since ``0.2.0``, so the field described a download that answers 404. It is dropped rather than repointed: a recording is addressed under its entity as ``/api/v1/{entity-type}/{id}/bulk-data/rosbags/{recording_id}``, and the entity type is part of the gateway's discovery model rather than anything the fault manager holds. The gateway already builds that URI itself, from the recording id it receives on the ``GetFault`` snapshot entries. +* The size a recording reports through ``GetFault``, ``GetSnapshots``, ``GetRosbag`` and ``ListRosbags`` is the storage file a download transfers, read from the bag's own ``metadata.yaml``, instead of the bag directory's total. The two differ by ``metadata.yaml``, which is not served, so every listing used to overstate its own download. The stored ``size_bytes`` and the ``snapshots.rosbag.max_total_storage_mb`` quota are unchanged and still count the whole directory, which is what eviction frees. A recording whose metadata cannot be read, and one split across several storage files past ``snapshots.rosbag.max_bag_size_mb``, both report the directory total as before. + 0.7.0 (2026-08-27) ------------------ * Rosbag black-box recordings are no longer limited to one per fault code. A fault that re-confirms keeps a bounded history of recordings instead of overwriting the previous one, controlled by the new ``snapshots.rosbag.max_bags_per_fault`` (default ``1``, which reproduces the previous behaviour exactly; ``0`` = unlimited). Retention is keep-newest and the bag is unlinked only when no fault still references it, so a burst that shares one recording behaves as before. Internally the ``rosbag_files`` grain changed from "one row per fault" to "one row per (fault, recording) link": ``recording_id`` is now a stored, indexed column, and the legacy column-level ``UNIQUE(fault_code)`` is replaced by a ``UNIQUE INDEX`` on ``(fault_code, file_path)`` through an automatic, idempotent table rebuild on first open. Four latent defects are fixed on the way: quota eviction deleted by fault code rather than by recording, ``get_rosbag_file`` had no ``ORDER BY`` and would have served an arbitrary recording, the stale-row self-heals deleted a fault's entire history because one bag had vanished from disk, and both ``delete_rosbag_file`` / ``delete_rosbag_files`` read only the first ``file_path`` of a fault, so deleting a fault with several recordings removed every row but left all but one bag on disk - unreachable and still charged against the quota (`#623 `_, `#620 `_) 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..bae9c3cde 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 += static_cast(entry.file_size()); + } + } + served = static_cast(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. From b282e2a8764ab068d7eb99600229cca7188eb7a0 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 13:51:33 +0200 Subject: [PATCH 4/9] fix(gateway): keep a split recording and an unreadable bag out of the listing's way Three defects in the descriptor sizing added on this branch, all of which reach a user. A recording split across several storage files was listed at the size of whichever segment the directory walk reached first. The rule the REST reference states, and the one the fault manager already follows, is that a split recording reports its total, because the download hands over one segment and no single file describes the transfer. The gateway now decides "split" the way the fault manager decides it, by reading relative_file_paths out of the bag's own metadata.yaml, and declines to answer for anything but a single storage file. The descriptor then keeps the row's figure, which is the fault manager's answer to the same question, so the two API surfaces agree on one recording again. One unreadable bag directory took the whole listing with it. The resolver used the throwing filesystem overloads, so EACCES on a single directory, or ENOENT when quota eviction removed one mid-walk, threw out of the listing handler, which has no catch in its chain, and the request answered 500 with every other recording of that entity gone. Before this branch the listing touched no filesystem at all, so the blast radius was one download. Every filesystem call in the resolver and the helper now takes the error_code overload, an error is an unmeasured recording rather than a failed request, and the download route gets the same protection since it shares the resolver. The README still told a reader to untar the download and play the extracted directory. The download is one storage file streamed verbatim, so tar answered "not in gzip format" and ros2 bag play answered that the path does not exist. The section now documents what was run against a real recording of each storage format: ros2 bag info and ros2 bag play, pointed straight at the downloaded file, with no unpacking step and no --storage flag. Also: the fallback comment said the row carries the bag directory's total, which stopped being true when the fault manager began sending served bytes. The Postman collection aimed three requests at the snapshot routes removed in 0.2.0, and is dropped rather than repointed because the collection has no bulk-data section to mirror. The size rule in the REST reference gains the clause a client needs, that size exceeding Content-Length is how a partial download of a split recording can be recognised, and is now referenced from the download headers instead of being an unused label. The integration suite now checks a descriptor's size against the bytes its own download delivers, which nothing connected before. --- docs/api/rest.rst | 16 +- ...os2-medkit-gateway.postman_collection.json | 68 ------- src/ros2_medkit_gateway/README.md | 36 +++- .../core/http/handlers/bulkdata_handlers.hpp | 35 ++-- .../src/http/handlers/bulkdata_handlers.cpp | 120 +++++++++-- .../test/test_bulkdata_handlers.cpp | 189 +++++++++++++++++- .../test/features/test_bulk_data_api.test.py | 45 +++++ 7 files changed, 398 insertions(+), 111 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 762416032..39a691e7c 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,8 @@ 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:`rest-recording-size-rule` - ``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_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..577fa80ce 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 @@ -216,21 +216,32 @@ 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. + * ``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. * - * 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. + * 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. + * + * 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 + * @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..6190fee1e 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" @@ -87,21 +89,41 @@ 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 rather than style. 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 ""; + } + 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(); } } @@ -122,10 +144,71 @@ std::string rosbag_recording_id(const std::string & file_path) { return p.filename().string(); } +namespace { + +/// How many storage files the bag at @p bag_path recorded, according to its own +/// ``metadata.yaml``. +/// +/// Decided from the metadata rather than by counting ``.db3`` / ``.mcap`` entries +/// on disk for one reason: the fault manager decides the same question the same +/// way (`rosbag_served_bytes` there reads `relative_file_paths` through +/// `rosbag2_storage::MetadataIo`), and the two API surfaces have to agree on +/// whether a given recording is split. Counting files on disk would disagree the +/// moment a segment of a split were deleted - the directory would then hold one +/// file and this side would call the recording whole while the fault manager +/// still called it split. The gateway already links yaml-cpp, so reading the +/// metadata costs no new dependency. It does not link rosbag2_storage, which is +/// why the field is read directly instead of through MetadataIo. +/// +/// nullopt when the metadata is missing, unreadable or not the shape rosbag2 +/// writes - all of which mean the same thing here, that this side cannot tell. +std::optional rosbag_storage_file_count(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; + } + return paths.size(); + } catch (const std::exception &) { + return std::nullopt; + } +} + +} // namespace + std::optional rosbag_served_bytes(const std::string & bag_path) { if (bag_path.empty()) { return std::nullopt; } + + // 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. Answering with the segment the + // resolver happened to reach first advertised a split recording at the size of + // one part of it, and disagreed with the fault manager, which reports the + // recording's total for a split. 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. + const auto storage_files = rosbag_storage_file_count(bag_path); + if (!storage_files || *storage_files != 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. @@ -226,13 +309,16 @@ 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. Listing + // a zero instead would describe the recording as empty rather than as + // unmeasured here. 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; diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index d3ba1048b..43081db1e 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -267,7 +267,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 +280,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 +321,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. @@ -336,6 +355,170 @@ TEST_F(RosbagBagDirectoryTest, ServedBytesIsUnknownRatherThanZeroWhenTheBagIsNot 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 rather than guessing, which is what makes the fallback fire. + EXPECT_FALSE(handlers::detail::rosbag_served_bytes(split_dir.string()).has_value()); +} + +// 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 is a symlink loop rather than a 0000 directory because it has to +// be refused for any uid, and these tests do not run under one uid. In CI they +// run as root: the workflow's jobs declare a plain `container:` with no `user:` +// key, and that runs as uid 0. Locally they run as uid 1000. Root ignores mode +// bits, so a 0000 directory is readable in CI and a test built on one would pass +// there without the failure it claims to reproduce. ELOOP is refused for every +// uid alike, so this case means the same thing in both places. +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; + std::filesystem::is_directory(loop_, probe_ec); + ASSERT_TRUE(static_cast(probe_ec)) << "the symlink loop resolved, so it is not an unreadable bag"; + + 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 actually pin the throwing overloads, and the + // listing assertion above is defence in depth rather than the guard. 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()), ""); }); +} + +// The second shape: a directory whose mode denies everyone. This one does test +// something under uid 1000, where it runs today, and cannot under uid 0, where CI +// runs it. It probes first and skips with the uid rather than passing on a +// permission that was never actually denied. +TEST_F(UnreadableBagTest, AModeZeroDirectoryIsAlsoDeclinedRatherThanThrown) { + 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'); + } + std::error_code ec; + std::filesystem::permissions(locked, std::filesystem::perms::none, ec); + ASSERT_FALSE(static_cast(ec)) << "could not drop the directory's permissions"; + + std::error_code probe_ec; + std::filesystem::directory_iterator probe(locked, probe_ec); + if (!probe_ec) { + std::filesystem::permissions(locked, std::filesystem::perms::owner_all, ec); + GTEST_SKIP() << "running as uid " << ::getuid() << ", which ignores mode bits, so a 0000 directory is readable. " + << "The symlink-loop case above is the one that covers every uid"; + } + + EXPECT_NO_THROW({ EXPECT_FALSE(handlers::detail::rosbag_served_bytes(locked.string()).has_value()); }); + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(locked.string()), ""); }); + + std::filesystem::permissions(locked, std::filesystem::perms::owner_all, ec); +} + TEST_F(RosbagBagDirectoryTest, AnUnreachableBagKeepsTheStoredFigureRatherThanReportingZero) { const json row{{"fault_code", "MOTOR_OVERHEAT"}, {"recording_id", "fault_MOTOR_OVERHEAT_1738664999000"}, 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 fd725118bacc9370bd626c87d4409eab6f137f18 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 14:04:51 +0200 Subject: [PATCH 5/9] docs: give the size-rule cross reference its own link text The label sits on a paragraph rather than a section title, so a bare :ref: has no title to take its link text from and sphinx -W fails the build with "Failed to create a cross reference. A title or caption not found". Naming the text explicitly is the form that works for a label on a paragraph. --- docs/api/rest.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 39a691e7c..47e308823 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1815,7 +1815,8 @@ Download a specific bulk-data file. 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:`rest-recording-size-rule` + 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`` From 5ba77f6812dad4ce6126f6c8a3d5e52cba62aa9e Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 9 Sep 2026 14:35:51 +0200 Subject: [PATCH 6/9] fix(gateway): let the bag name its own storage file, and size a bare one Three follow-ups to the split fix, plus the comments it left behind. A row's file_path can be the storage file itself rather than a bag directory. The resolver has always accepted that and the download serves it, but the single-file gate asked the path's metadata.yaml, which a bare file does not have, so the listing declined every such row. Harmless while the row carries a figure to fall back on, and a recording listed at zero the moment one does not. A path that is already a storage file is one storage file, and is now sized as it stands. When the metadata names exactly one file, the resolver now returns that file instead of whichever .db3 or .mcap the directory iterator yields first. A stray file beside the recording, a leftover segment or a copy, could otherwise be served and sized in place of the real one while the fault manager, which sizes relative_file_paths.front(), reported the other: on a directory holding a named 4096-byte recording and a 65536-byte stray, the gateway listed and would serve 65536 against the fault manager's 4096. The fix is in the resolver, so the listing and the download move together, and both sides now read the same field. A recording whose metadata names several files is untouched, that being the split case. The comments above the resolver call and on its declaration still claimed the listing and the download always resolve the same file. That holds for a recording in one storage file and deliberately does not for a split, where the listing carries the recording's total and the download hands over one file. Both now say so, and the reason on the empty-directory case says what it exercises now, which is the metadata gate rather than the resolver. Also: a nodiscard is_directory result is used rather than discarded. --- .../test/test_rosbag_capture.cpp | 22 +++ .../core/http/handlers/bulkdata_handlers.hpp | 32 +++- .../src/http/handlers/bulkdata_handlers.cpp | 143 +++++++++++------- .../test/test_bulkdata_handlers.cpp | 72 ++++++++- 4 files changed, 208 insertions(+), 61 deletions(-) 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..8fa660892 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/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 577fa80ce..167a5eb3a 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 rather than being a private helper of the + * download path. `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 @@ -217,7 +230,9 @@ 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. * * Answers only for a recording held in a single storage file, which is the only - * shape where one number describes the transfer. + * 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 @@ -239,7 +254,8 @@ bool rosbag_resolved_by_fault_code(const nlohmann::json & rosbag_data, const std * 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) + * @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 */ 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 6190fee1e..730103a70 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -65,6 +65,53 @@ 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 what the fault +/// manager reads for the same decisions (through +/// ``rosbag2_storage::MetadataIo``). Reading it here rather than inferring the +/// answer from what happens to sit in the directory is what keeps the two sides +/// agreeing about one recording. The gateway already links yaml-cpp, so this +/// costs no new dependency. It does not link rosbag2_storage, which is why the +/// field is read directly instead of through MetadataIo. +/// +/// 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; + } +} + } // namespace BulkDataHandlers::BulkDataHandlers(HandlerContext & ctx) : ctx_(ctx) { @@ -108,6 +155,27 @@ std::string BulkDataHandlers::resolve_rosbag_file_path(const std::string & path) 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 returned whichever .db3 or .mcap the directory happened to + // yield first, so a stray file beside the recording - a leftover segment, a + // copy - could be served and sized in place of the real one, while the fault + // manager, which sizes relative_file_paths.front(), reported the other. Same + // question, same evidence, on both sides now. + // + // 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, or 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) { + const std::filesystem::path named = std::filesystem::path(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 ""; @@ -144,56 +212,14 @@ std::string rosbag_recording_id(const std::string & file_path) { return p.filename().string(); } -namespace { - -/// How many storage files the bag at @p bag_path recorded, according to its own -/// ``metadata.yaml``. -/// -/// Decided from the metadata rather than by counting ``.db3`` / ``.mcap`` entries -/// on disk for one reason: the fault manager decides the same question the same -/// way (`rosbag_served_bytes` there reads `relative_file_paths` through -/// `rosbag2_storage::MetadataIo`), and the two API surfaces have to agree on -/// whether a given recording is split. Counting files on disk would disagree the -/// moment a segment of a split were deleted - the directory would then hold one -/// file and this side would call the recording whole while the fault manager -/// still called it split. The gateway already links yaml-cpp, so reading the -/// metadata costs no new dependency. It does not link rosbag2_storage, which is -/// why the field is read directly instead of through MetadataIo. -/// -/// nullopt when the metadata is missing, unreadable or not the shape rosbag2 -/// writes - all of which mean the same thing here, that this side cannot tell. -std::optional rosbag_storage_file_count(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; - } - return paths.size(); - } catch (const std::exception &) { - return std::nullopt; - } -} - -} // namespace - 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 @@ -204,9 +230,18 @@ std::optional rosbag_served_bytes(const std::string & bag_path) { // 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. - const auto storage_files = rosbag_storage_file_count(bag_path); - if (!storage_files || *storage_files != 1) { - return std::nullopt; + // + // 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 has always accepted a + // bare file, and the download serves it. Checking the count first made the + // helper decline every such row, and a row that carried no figure would then + // have been listed at zero. + if (!is_storage_file) { + const auto 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 @@ -605,9 +640,15 @@ 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 held in one storage file, 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. For a recording split + // across several files the two deliberately differ: the listing carries the + // recording's total, this route hands over one file, and the descriptor size + // exceeding Content-Length is how a client can tell the transfer is partial. + // 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 43081db1e..9395c4e2f 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -349,7 +349,11 @@ 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, but that is no longer what the test 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()); @@ -395,6 +399,69 @@ TEST_F(RosbagBagDirectoryTest, ASplitRecordingIsListedAtTheRowsFigureNotAtOneSeg 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 rather than a bag + // directory. The resolver has always accepted that 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 consulting the metadata first made the helper decline + // every such row - harmless while the row carries a figure to fall back on, and + // a recording listed at zero the moment one does not. + 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 - used to be + // servable and sizeable in place of the real one, because the resolver took + // whichever file the directory iterator yielded first. The fault manager sizes + // relative_file_paths.front() (rosbag_capture.cpp, rosbag_served_bytes), so the + // two sides reported different numbers for the same directory. The bag's own + // metadata is the tie-break on both sides now. + 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')); + write_metadata(strays, {"recording_0.db3"}); + + const uint64_t named_size = std::filesystem::file_size(strays / "recording_0.db3"); + const uint64_t stray_size = std::filesystem::file_size(strays / "recording_1.db3"); + 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()), (strays / "recording_0.db3").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); + // 4096 is also what the fault manager's own helper answers for this directory + // shape, which is the point of reading the same field on both sides. Its + // behaviour is pinned by ReportsTheStorageFileNotTheDirectoryTotal in + // test_rosbag_capture.cpp. + EXPECT_EQ(descriptors[0].size, named_size) << "the listing must report the file the bag names"; + EXPECT_EQ(descriptors[0].size, 4096u) << "and that is the number the fault manager reports too"; + EXPECT_NE(descriptors[0].size, stray_size) << "directory order must not decide which file a recording is"; +} + // 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 @@ -453,8 +520,9 @@ 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; - std::filesystem::is_directory(loop_, 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"); From acc12db57334bda519cd79bfc21c4d33f4c952b3 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 10 Sep 2026 09:58:12 +0200 Subject: [PATCH 7/9] fix(gateway): download a split recording's first file, and say how many there are Past snapshots.rosbag.max_bag_size_mb a recording is held in several storage files rather than one, and the download route hands over a single file. Which file that was came from the bag directory's own listing order, so a client downloading a split recording received a segment from the middle of it as readily as its start, and which segment could differ between two requests for the same recording on the same host. The recording's metadata.yaml names its storage files in capture order, so the first name in it is where the recording starts, and the resolver now takes that one. A named file that is not on disk is skipped in favour of the next one that is: quota eviction and a half-copied bag both leave metadata naming a file that is gone, and resolving to it would answer 500 for a recording whose remaining segments are readable. Only when the bag will not say at all is the directory scanned as before. For a recording held in one file this is the rule that was already there, widened rather than changed. Receiving one segment was also silent about the rest. The rosbag descriptor now carries x-medkit.storage_files, how many storage files the recording holds, read from the same metadata field that picks the served file so the count and the choice cannot describe different recordings. It is omitted when that metadata cannot be read, because a 1 there would claim a recording is whole on the evidence of nothing, which is the one thing the field exists to establish. The descriptor size is untouched and still reports the whole recording for a split, which is why it exceeds the download's Content-Length. The OpenAPI document declares x-medkit as a free-form object and enumerates no fields inside it, so it needed no change. --- docs/api/rest.rst | 40 ++- src/ros2_medkit_gateway/CHANGELOG.rst | 4 + src/ros2_medkit_gateway/README.md | 9 +- .../core/http/handlers/bulkdata_handlers.hpp | 58 +++- .../ros2_medkit_gateway/dto/bulkdata.hpp | 15 +- .../src/http/handlers/bulkdata_handlers.cpp | 81 +++++- .../test/test_bulkdata_handlers.cpp | 259 ++++++++++++++++++ 7 files changed, 427 insertions(+), 39 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 47e308823..68182a22e 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1758,7 +1758,8 @@ List all bulk-data items in a category for the entity. "fault_codes": ["MOTOR_OVERHEAT", "MOTOR_STALL"], "duration_sec": 6.0, "format": "mcap", - "recording_id": "fault_MOTOR_OVERHEAT_1738664999000" + "recording_id": "fault_MOTOR_OVERHEAT_1738664999000", + "storage_files": 1 } } ] @@ -1772,6 +1773,13 @@ 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. +``x-medkit.storage_files`` is how many storage files the recording is held in, +read from the recording's own metadata. A ``1`` is the normal case and says the +download below hands over the whole recording. A larger number is a split +recording, and then the download hands over the first of those files. The field +is omitted when the gateway cannot read the recording's metadata, so an absent +field means "not known here" rather than "one". + ``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, @@ -1792,12 +1800,24 @@ 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. +**A split recording.** Past the configured maximum bag size a recording is held +in several storage files rather than one. Three things are then true at once, +and they are meant to be read together: + +- the download hands over the **first** storage file of the recording, the one + its own metadata names first, which is where the recording starts. Which file + that is does not depend on the host's filesystem and does not change between + two requests for the same recording, +- ``x-medkit.storage_files`` says **how many** storage files there are, so a + client can tell it received a part and know how many parts the recording has, +- ``size`` stays the **whole recording**, as does the nested + ``environment_data.snapshots[].size_bytes``, while the download's + ``Content-Length`` is that one file, so ``size`` exceeds ``Content-Length``. + +That last gap is a second signal for the same fact and is kept because a client +that has already made the transfer can read it without listing anything. The +storage files of a split recording after the first are not addressable over the +API today. Download Bulk Data ~~~~~~~~~~~~~~~~~~ @@ -1814,8 +1834,10 @@ 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 +- ``Content-Length``: the served file's length. For a rosbag that is the + recording's single storage file, or, when the recording is split, the first + of its storage files. For how it relates to the descriptor ``size`` and + ``x-medkit.storage_files`` 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 diff --git a/src/ros2_medkit_gateway/CHANGELOG.rst b/src/ros2_medkit_gateway/CHANGELOG.rst index 6c4dd39ee..064e9ae69 100644 --- a/src/ros2_medkit_gateway/CHANGELOG.rst +++ b/src/ros2_medkit_gateway/CHANGELOG.rst @@ -2,6 +2,10 @@ Changelog for package ros2_medkit_gateway ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* Downloading a rosbag recording that is split across several storage files, past ``snapshots.rosbag.max_bag_size_mb``, now hands over the first of them - the one the recording's own ``metadata.yaml`` names first, which is where the recording starts - instead of whichever ``.db3`` or ``.mcap`` the gateway host's directory listing happened to yield first. Which segment a client received was previously decided by the filesystem and could differ between two requests for the same recording. The rosbag descriptor gains ``x-medkit.storage_files``, the number of storage files the recording holds, so a client can tell a whole recording from one part of a split one. It is omitted when the gateway cannot read the recording's metadata, so an absent field means "not known here" rather than "one". The descriptor ``size`` is unchanged and still reports the whole recording for a split, which is why it exceeds the download's ``Content-Length``. + 0.7.0 (2026-08-27) ------------------ * Rosbag bulk-data is addressed by recording id instead of fault code, so a fault holding several recordings can expose each one. ``GET /{entity}/bulk-data/rosbags`` now emits one descriptor per recording rather than one per fault - a burst that shares a bag used to appear as several entries each reporting the full bag size - and the covered faults move into ``x-medkit.fault_codes`` (was the scalar ``x-medkit.fault_code``). Old URLs keep working: an id that is not a recording is resolved as a fault code and serves that fault's newest recording, which is what it returned before. Authorization is unchanged in effect - a download is allowed when any fault the recording covers is in the entity's source scope, which is exactly the set that could reach it previously (`#623 `_, `#620 `_) diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index 8227c874f..c62f8c21c 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1165,10 +1165,11 @@ download serves one storage file, verbatim, named `.` 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. +across several storage files, and then the download hands over the first of +them, `x-medkit.storage_files` says how many there are, and `size` stays the +whole recording, so it 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:** 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 167a5eb3a..d1d00f2a0 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,7 @@ #pragma once +#include #include #include #include @@ -123,11 +124,19 @@ class BulkDataHandlers { * 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``. + * transfer is partial, alongside ``x-medkit.storage_files`` in the descriptor. + * 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. + * The bag's own ``metadata.yaml`` decides: the first file it names in + * ``relative_file_paths`` that is on disk is the answer. That order is the + * capture order, so for a split recording the first name is where the + * recording begins and a client that fetches one gets its start rather than an + * arbitrary slice. A name that is not on disk is skipped, because quota + * eviction and a half-copied bag both leave metadata naming a file that is + * gone and the route would answer 500 on a recording whose other segments are + * readable. Only when the bag will not say - no metadata, unreadable metadata, + * or not one named file present - is the directory 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 @@ -226,6 +235,39 @@ 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 How many storage files one recording is held in. + * + * Read from the bag's own ``metadata.yaml``, the same field + * ``BulkDataHandlers::resolve_rosbag_file_path`` picks the served file out of, + * so the count and the choice of segment cannot describe different recordings. + * A @p bag_path that is itself a storage file is one by definition and carries + * no metadata beside it under that name to consult. + * + * It reaches the client as ``x-medkit.storage_files`` on the rosbag descriptor, + * and what it is for is the split case. There the descriptor ``size`` is the + * whole recording while the download hands over one segment, so ``size`` and + * ``Content-Length`` differ. Without this field that difference has no stated + * reason, and a client holding one segment has no way to learn that the rest of + * the recording exists. A ``1`` says the transfer was the whole recording. + * + * nullopt when the bag will not say - no metadata, unreadable metadata, or not + * the shape rosbag2 writes - and the field is then omitted from the descriptor + * rather than defaulted. Counting the directory's ``.db3`` / ``.mcap`` files + * instead would count a stray beside the recording, and defaulting to one would + * claim a recording is whole on the evidence of nothing. + * + * Never throws, for the same reason as the two helpers around it: it runs once + * per row of a listing, and one unreadable recording must not cost the entity's + * other recordings. + * + * @param bag_path Bag path as stored by the fault manager. A bag directory, or + * a bare storage file, which both answer + * @return The number of storage files the recording names, or nullopt when the + * bag's metadata cannot be read + */ +std::optional rosbag_storage_file_count(const std::string & bag_path); + /** * @brief Bytes a rosbag download puts on the wire for one recording. * @@ -244,8 +286,8 @@ bool rosbag_resolved_by_fault_code(const nlohmann::json & rosbag_data, const std * 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 + * the transfer, and answering with the segment it hands over 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. @@ -281,6 +323,10 @@ std::optional rosbag_served_bytes(const std::string & bag_path); * 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. * + * ``x-medkit.storage_files`` carries how many files the recording is held in + * (see ``rosbag_storage_file_count``), and is omitted for a bag whose metadata + * this process cannot read. + * * @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 9c8ca8acb..b94ea81ed 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 @@ -56,13 +56,18 @@ inline constexpr std::string_view dto_name = "BulkDataCate // name - human-readable filename / label (required) // mimetype - MIME type of the file (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 +// (required). For a rosbag held in one storage file that is +// the file, not the bag directory's total. For one split +// across several it is the whole recording, which the +// download exceeds by handing over the first file alone // creation_date - ISO 8601 timestamp string (required) // description - optional human-readable description -// x-medkit - optional open vendor extension object; for rosbags: -// {fault_code, duration_sec, format}; for user uploads: -// arbitrary metadata JSON object set by the uploader. +// x-medkit - optional open vendor extension object. For rosbags: +// {fault_codes, duration_sec, format, recording_id, +// storage_files}, the last of which is how many storage +// files the recording holds and is omitted when the bag's +// metadata cannot be read. For user uploads: an arbitrary +// metadata JSON object set by the uploader. // ============================================================================= struct BulkDataDescriptor { std::string id; 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 730103a70..c51ae7a0f 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -156,23 +156,38 @@ std::string BulkDataHandlers::resolve_rosbag_file_path(const std::string & path) 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 returned whichever .db3 or .mcap the directory happened to - // yield first, so a stray file beside the recording - a leftover segment, a - // copy - could be served and sized in place of the real one, while the fault + // Ask the bag first. Its metadata names the storage files it holds, in the + // order they were recorded, and the first of those that is on disk is the file + // to hand over. Directory order does not get a vote. + // + // For a recording held in one file that removes a stray .db3 beside it - a + // leftover segment, a copy - from being served and sized in place of the real + // one, which is what the directory walk below could pick while the fault // manager, which sizes relative_file_paths.front(), reported the other. Same - // question, same evidence, on both sides now. + // question, same evidence, on both sides. + // + // For a recording split across several files it decides which segment the + // download is. The walk below answered with whichever .db3 or .mcap the + // directory happened to yield first, which is a segment from the middle of the + // recording as readily as its start, and which file that was could change + // between two requests for the same bag. The metadata's order is the capture + // order, so its first name is where the recording begins, and a client that + // fetches a split recording gets its start rather than an arbitrary slice. + // ``x-medkit.storage_files`` in the descriptor is what tells that client the + // rest of the recording exists (see rosbag_storage_file_count). // - // 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, or 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) { - const std::filesystem::path named = std::filesystem::path(path) / names->front(); - std::error_code named_ec; - if (std::filesystem::is_regular_file(named, named_ec) && !named_ec) { - return named.string(); + // A name that is not on disk is skipped rather than returned: quota eviction + // and a half-copied bag both leave metadata naming a file that is gone, and + // resolving to it would answer 500 for a recording whose other segments are + // readable. When the bag says nothing at all - no metadata, unreadable + // metadata, or not one named file present - the walk below is the fallback. + if (const auto names = rosbag_relative_file_paths(path); names) { + for (const auto & name : *names) { + const std::filesystem::path named = std::filesystem::path(path) / name; + std::error_code named_ec; + if (std::filesystem::is_regular_file(named, named_ec) && !named_ec) { + return named.string(); + } } } @@ -212,6 +227,30 @@ std::string rosbag_recording_id(const std::string & file_path) { return p.filename().string(); } +std::optional rosbag_storage_file_count(const std::string & bag_path) { + if (bag_path.empty()) { + return std::nullopt; + } + + // A path that is already a storage file is one storage file, and has no + // metadata.yaml beside it under that name to ask. The resolver accepts such a + // path and the download serves it, so it is a shape the descriptor describes. + std::error_code ec; + if (std::filesystem::is_regular_file(bag_path, ec) && !ec) { + return std::size_t{1}; + } + + // Anything else is answered by the bag's own record or not at all. A directory + // walk would count strays beside the recording, and a bag this process cannot + // read would count zero, which reads as an empty recording rather than as an + // unread one. + const auto names = rosbag_relative_file_paths(bag_path); + if (!names) { + return std::nullopt; + } + return names->size(); +} + std::optional rosbag_served_bytes(const std::string & bag_path) { if (bag_path.empty()) { return std::nullopt; @@ -295,6 +334,7 @@ fold_rosbag_rows_into_descriptors(const std::vector & rows, std::string recording_id; std::string format; uint64_t size_bytes{0}; + std::optional storage_files; double duration_sec{0.0}; int64_t created_at_ns{0}; std::vector fault_codes; @@ -355,6 +395,11 @@ fold_rosbag_rows_into_descriptors(const std::vector & rows, // a zero instead would describe the recording as empty rather than as // unmeasured here. entry.size_bytes = rosbag_served_bytes(row.value("file_path", "")).value_or(row.value("size_bytes", uint64_t{0})); + // How many storage files the recording holds, so a client can tell a whole + // recording from one segment of a split. Without it the descriptor size and + // the download's Content-Length differ with no stated reason, and a client + // that fetched a split has no way to learn that more of it exists. + entry.storage_files = rosbag_storage_file_count(row.value("file_path", "")); entry.duration_sec = row.value("duration_sec", 0.0); entry.created_at_ns = created_at_ns; entry.fault_codes.push_back(fault_code); @@ -380,6 +425,12 @@ fold_rosbag_rows_into_descriptors(const std::vector & rows, // Redundant with the descriptor id, kept because // clients already group on it. {"recording_id", entry.recording_id}}; + // Omitted rather than defaulted when the bag would not say: a one there + // would claim the recording is whole, which is the one thing this field + // exists to establish. + if (entry.storage_files) { + (*descriptor.x_medkit)["storage_files"] = *entry.storage_files; + } descriptors.push_back(std::move(descriptor)); } return descriptors; diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index 9395c4e2f..190561d03 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -299,6 +299,30 @@ class RosbagBagDirectoryTest : public ::testing::Test { write_file(dir / "metadata.yaml", yaml); } + /// The storage file the directory-order fallback inside + /// ``resolve_rosbag_file_path`` reaches first: the first regular ``.db3`` or + /// ``.mcap`` the directory yields. Empty when the directory holds none. + /// + /// The split tests use this to choose a metadata order that cannot coincide + /// with the directory's. Which file a directory yields first is the + /// filesystem's own business - it is neither creation order nor lexical order + /// on the overlay these tests run on - so a test that writes down an expected + /// answer and hopes it differs from directory order proves nothing on the run + /// where the two agree. Asking at run time makes "the bag's own order decides" + /// falsifiable everywhere. + static std::filesystem::path first_in_directory_order(const std::filesystem::path & dir) { + for (const auto & entry : std::filesystem::directory_iterator(dir)) { + if (!entry.is_regular_file()) { + continue; + } + const auto ext = entry.path().extension().string(); + if (ext == ".db3" || ext == ".mcap") { + return entry.path(); + } + } + return {}; + } + // What the fault manager stores: every regular file under the bag directory. uint64_t directory_total() const { uint64_t total = 0; @@ -462,6 +486,241 @@ TEST_F(RosbagBagDirectoryTest, TheMetadataNamesTheStorageFileRatherThanDirectory EXPECT_NE(descriptors[0].size, stray_size) << "directory order must not decide which file a recording is"; } +// === A split recording: which segment is downloaded, and how many there are === +// Past the configured maximum bag size rosbag2 splits a recording across several +// storage files. The download hands over one of them, and that used to be +// whichever the directory iterator yielded first - a segment from the middle of +// the recording as readily as its start, decided by nothing a client could see +// or predict. The recording's own metadata.yaml lists its segments in capture +// order, so the first name in it is where the recording starts, and that is the +// file to hand over. The count goes into the descriptor because a client holding +// one segment has no other way to learn that more of the recording exists. + +TEST_F(RosbagBagDirectoryTest, ASplitRecordingResolvesToTheFirstSegmentTheMetadataNames) { + // Three segments, and the one the metadata names first is chosen so that + // neither of the rules this one replaces can reach it: it is not the file the + // directory yields first, and it is not the lexically smallest. So a resolver + // that walks the directory, and a resolver that sorts, both have to fail here. + const auto split_dir = bag_dir_ / "split_ordered"; + std::filesystem::create_directories(split_dir); + const std::vector segments{split_dir / "recording_0.db3", split_dir / "recording_1.db3", + split_dir / "recording_2.db3"}; + write_file(segments[0], std::string(16384, 'a')); + write_file(segments[1], std::string(53248, 'b')); + write_file(segments[2], std::string(32768, 'c')); + // Written before the order is read, and rewritten in place afterwards, so + // adding metadata.yaml cannot move the entries the answer was read from. + write_metadata(split_dir, {"recording_0.db3"}); + + const auto directory_first = first_in_directory_order(split_dir); + ASSERT_FALSE(directory_first.empty()) << "no storage file in the directory, so nothing is being told apart"; + const auto & lexically_first = segments.front(); + + std::filesystem::path metadata_first; + for (const auto & segment : segments) { + if (segment != directory_first && segment != lexically_first) { + metadata_first = segment; + break; + } + } + ASSERT_FALSE(metadata_first.empty()) << "three segments always leave one that is neither"; + + std::vector names{metadata_first.filename().string()}; + for (const auto & segment : segments) { + if (segment != metadata_first) { + names.push_back(segment.filename().string()); + } + } + write_metadata(split_dir, names); + + EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(split_dir.string()), metadata_first.string()) + << "the download must hand over the segment the recording names first, not the one the directory offers"; + + 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_named = std::filesystem::file_size(metadata_first); + + // 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); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + ASSERT_TRUE(descriptors[0].x_medkit->contains("storage_files")) << "a split recording must say how many files it has"; + EXPECT_EQ((*descriptors[0].x_medkit)["storage_files"], 3) << "the count is what the bag's own metadata names"; + EXPECT_EQ(descriptors[0].size, split_total) << "a split recording is still listed at the whole recording's size"; + EXPECT_NE(descriptors[0].size, first_named) << "the segment on the wire is not the recording"; +} + +TEST_F(RosbagBagDirectoryTest, ASplitRecordingSkipsAFirstSegmentThatIsNoLongerOnDisk) { + // Quota eviction and a half-copied bag both leave metadata naming a file that + // is gone. Resolving to a path that does not exist answers 500 for a recording + // whose remaining segments are readable, so the first name that IS on disk is + // served instead. The survivor named first is again the one the directory does + // not yield first, so directory order cannot produce this answer either. + const auto split_dir = bag_dir_ / "split_first_gone"; + std::filesystem::create_directories(split_dir); + const std::filesystem::path evicted = split_dir / "recording_0.db3"; + write_file(split_dir / "recording_1.db3", std::string(24576, 'c')); + write_file(split_dir / "recording_2.db3", std::string(40960, 'd')); + write_metadata(split_dir, {"recording_1.db3"}); + ASSERT_FALSE(std::filesystem::exists(evicted)) << "the first segment has to be missing"; + + const auto directory_first = first_in_directory_order(split_dir); + ASSERT_FALSE(directory_first.empty()); + const std::filesystem::path survivor = + directory_first == split_dir / "recording_1.db3" ? split_dir / "recording_2.db3" : split_dir / "recording_1.db3"; + + write_metadata(split_dir, + {evicted.filename().string(), survivor.filename().string(), directory_first.filename().string()}); + + EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(split_dir.string()), survivor.string()) + << "a named segment that is not on disk cannot be the one served, and the directory does not get the vote"; + + const json row{{"fault_code", "SPLIT_FAULT"}, + {"recording_id", "fault_SPLIT_FAULT_1738664999001"}, + {"file_path", split_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", 99999}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + ASSERT_TRUE(descriptors[0].x_medkit->contains("storage_files")); + EXPECT_EQ((*descriptors[0].x_medkit)["storage_files"], 3) + << "the count is what the recording holds, not what survived on disk"; +} + +TEST_F(RosbagBagDirectoryTest, ASplitRecordingWithNoSegmentLeftResolvesToNothing) { + // Nothing to hand over, and the download route turns an empty resolution into + // its 500. The count still answers, because the metadata is readable and it is + // the recording's own record of what it held. + const auto split_dir = bag_dir_ / "split_all_gone"; + std::filesystem::create_directories(split_dir); + write_metadata(split_dir, {"recording_0.db3", "recording_1.db3"}); + + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(split_dir.string()), ""); }); + + const json row{{"fault_code", "SPLIT_FAULT"}, + {"recording_id", "fault_SPLIT_FAULT_1738664999002"}, + {"file_path", split_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", 4242}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + ASSERT_TRUE(descriptors[0].x_medkit->contains("storage_files")); + EXPECT_EQ((*descriptors[0].x_medkit)["storage_files"], 2); + EXPECT_EQ(descriptors[0].size, 4242u) << "an unmeasurable recording keeps the figure its row carried"; +} + +TEST_F(RosbagBagDirectoryTest, StorageFileCountIsOneForAWholeRecordingAndAbsentWhenTheBagWillNotSay) { + // One has to be stated rather than left out, because an absent field already + // means something else here: that this side could not read the recording's + // metadata at all. A client cannot tell "one file" from "unknown" if both are + // silence. + const json whole_row{{"fault_code", "MOTOR_OVERHEAT"}, + {"recording_id", bag_dir_.filename().string()}, + {"file_path", bag_dir_.string()}, + {"format", "sqlite3"}, + {"size_bytes", directory_total()}}; + + const auto whole = handlers::detail::fold_rosbag_rows_into_descriptors({whole_row}, {}); + ASSERT_EQ(whole.size(), 1u); + ASSERT_TRUE(whole[0].x_medkit.has_value()); + ASSERT_TRUE(whole[0].x_medkit->contains("storage_files")) << "a whole recording states its one file"; + EXPECT_EQ((*whole[0].x_medkit)["storage_files"], 1); + + // A bare storage file is one storage file by definition, and has no + // metadata.yaml beside it under that name to consult. + const auto bare_file = bag_dir_ / "standalone.db3"; + write_file(bare_file, std::string(2048, 'z')); + const json bare_row{{"fault_code", "BARE"}, + {"recording_id", "standalone.db3"}, + {"file_path", bare_file.string()}, + {"format", "sqlite3"}, + {"size_bytes", 1}}; + const auto bare = handlers::detail::fold_rosbag_rows_into_descriptors({bare_row}, {}); + ASSERT_EQ(bare.size(), 1u); + ASSERT_TRUE(bare[0].x_medkit.has_value()); + ASSERT_TRUE(bare[0].x_medkit->contains("storage_files")); + EXPECT_EQ((*bare[0].x_medkit)["storage_files"], 1); + + // No metadata to read: the field is omitted rather than guessed at one. The + // assertions above are the positive control for this absence - the same helper + // on the same harness does emit the field when the bag answers. + const auto silent_dir = bag_dir_ / "no_metadata"; + std::filesystem::create_directories(silent_dir); + write_file(silent_dir / "recording_0.db3", std::string(1024, 'q')); + const json silent_row{{"fault_code", "SILENT"}, + {"recording_id", "no_metadata"}, + {"file_path", silent_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", 1024}}; + const auto silent = handlers::detail::fold_rosbag_rows_into_descriptors({silent_row}, {}); + ASSERT_EQ(silent.size(), 1u); + ASSERT_TRUE(silent[0].x_medkit.has_value()); + EXPECT_FALSE(silent[0].x_medkit->contains("storage_files")) << "a bag that will not say must not be counted at one"; +} + +TEST_F(RosbagBagDirectoryTest, TheDownloadAndTheListingAgreeOnWhichSegmentIsServed) { + // download() resolves the row's file_path through + // BulkDataHandlers::resolve_rosbag_file_path and reports that file's length as + // Content-Length. The call below is that one, with that argument, so the two + // sides cannot answer differently for one recording. What is pinned here is + // the pair: the transfer is the recording's first segment while the descriptor + // keeps the whole recording's size, and the gap between them is what a client + // reads as "this is a part". + const auto split_dir = bag_dir_ / "split_agreement"; + std::filesystem::create_directories(split_dir); + write_file(split_dir / "recording_0.db3", std::string(16384, 'a')); + write_file(split_dir / "recording_1.db3", std::string(53248, 'b')); + write_metadata(split_dir, {"recording_0.db3"}); + + const auto directory_first = first_in_directory_order(split_dir); + ASSERT_FALSE(directory_first.empty()); + const std::filesystem::path metadata_first = + directory_first == split_dir / "recording_0.db3" ? split_dir / "recording_1.db3" : split_dir / "recording_0.db3"; + write_metadata(split_dir, {metadata_first.filename().string(), directory_first.filename().string()}); + + 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 std::string served_path = BulkDataHandlers::resolve_rosbag_file_path(split_dir.string()); + ASSERT_EQ(served_path, metadata_first.string()); + std::error_code ec; + const uint64_t content_length = std::filesystem::file_size(served_path, ec); + ASSERT_FALSE(static_cast(ec)); + + const json row{{"fault_code", "SPLIT_FAULT"}, + {"recording_id", "fault_SPLIT_FAULT_1738664999003"}, + {"file_path", split_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", split_total}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(content_length, std::filesystem::file_size(metadata_first)) << "the transfer is that segment, whole"; + EXPECT_GT(descriptors[0].size, content_length) << "size exceeding Content-Length is how a client sees a split"; + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + ASSERT_TRUE(descriptors[0].x_medkit->contains("storage_files")); + EXPECT_EQ((*descriptors[0].x_medkit)["storage_files"], 2) << "and the count tells it how many there were"; +} + // 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 From 30f346d9d261a39c39194cd057758f3eead3e315 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 10 Sep 2026 10:21:49 +0200 Subject: [PATCH 8/9] docs(gateway): changelog for the descriptor size change The bulk-data descriptor sizing work has no changelog entry of its own: the package had no Forthcoming section until the split-recording entry above it created one. One bullet for what a reader of this package sees change, which is the rosbag descriptor size becoming the bytes the download serves, a bag naming its own storage file, an unreadable bag directory costing its own row instead of the listing, and the README dropping two routes that have answered 404 since 0.2.0. --- src/ros2_medkit_gateway/CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ros2_medkit_gateway/CHANGELOG.rst b/src/ros2_medkit_gateway/CHANGELOG.rst index 064e9ae69..20e546ba9 100644 --- a/src/ros2_medkit_gateway/CHANGELOG.rst +++ b/src/ros2_medkit_gateway/CHANGELOG.rst @@ -5,6 +5,7 @@ Changelog for package ros2_medkit_gateway Forthcoming ----------- * Downloading a rosbag recording that is split across several storage files, past ``snapshots.rosbag.max_bag_size_mb``, now hands over the first of them - the one the recording's own ``metadata.yaml`` names first, which is where the recording starts - instead of whichever ``.db3`` or ``.mcap`` the gateway host's directory listing happened to yield first. Which segment a client received was previously decided by the filesystem and could differ between two requests for the same recording. The rosbag descriptor gains ``x-medkit.storage_files``, the number of storage files the recording holds, so a client can tell a whole recording from one part of a split one. It is omitted when the gateway cannot read the recording's metadata, so an absent field means "not known here" rather than "one". The descriptor ``size`` is unchanged and still reports the whole recording for a split, which is why it exceeds the download's ``Content-Length``. +* The ``size`` a rosbag descriptor reports under ``GET /{entity}/bulk-data/rosbags`` is the storage file the download serves, measured through the same resolver the download uses, instead of the bag directory's total. The two differ by ``metadata.yaml``, which is never served, so every listing used to overstate its own download, 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. A bag whose ``metadata.yaml`` names exactly one storage file now resolves to that file rather than to whichever ``.db3`` or ``.mcap`` the directory yielded first, so a stray file beside the recording can no longer be served and sized in place of the real one, and the gateway and the fault manager decide it from the same field. A recording split across several storage files keeps the whole recording's figure, which is what the fault manager reports for it. A bag directory the gateway cannot read now costs its own row rather than the whole request: the resolver used the throwing filesystem calls, so one unreadable directory, or one removed by quota eviction mid-walk, answered ``500`` and took every other recording of that entity out of the listing with it. The gateway README no longer documents ``GET /faults/{code}/snapshots`` and ``.../snapshots/bag``, which have answered ``404`` since ``0.2.0``, because snapshots are returned inline with the fault and recordings are downloaded through the bulk-data endpoints. 0.7.0 (2026-08-27) ------------------ From 3fea8bb6c0b35c0fe0d73dc92daadc8f182eccb2 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 10 Sep 2026 10:52:04 +0200 Subject: [PATCH 9/9] fix(gateway): let a bag that named its files be the whole answer Three defects in the split-recording resolution, each of which can put a file on the wire that the recording does not contain. When a bag's metadata.yaml named its storage files and none of them was on disk, the resolver walked the bag directory and served whatever .db3 or .mcap it found there. On a directory holding a recording whose files are gone and one stray file beside them, the download served the stray under that recording's id, and the descriptor counted it in x-medkit.storage_files against a list the served file is not a member of. The same happened for a recording held in one file whose only named file was gone. A client received neither the recording nor an error, and nothing in the response said which. A bag that has named files is now the whole answer: the first name that is on disk, or nothing. The listing then keeps the row's own figure and the download answers its own error. A relative_file_paths naming nothing was reported as storage_files: 0, which is the value the field exists to avoid, because it describes the recording as empty rather than as one this side could not read. An empty list is a bag that did not answer, so it is declined like unreadable metadata, and the resolver falls back to the directory walk for it as it does for a bag with no metadata at all. That keeps the invariant the two helpers hold together: the count and the served file both come from the metadata, or both from the directory, never one from each. The names are joined onto the bag path. An absolute name replaces that path outright and a name climbing through .. walks out of the directory, so either one resolved to a file outside the recording and the download streamed it under the recording's id. Both are skipped now. rosbag2 writes plain basenames, so nothing legitimate is refused. Also: the REST reference, the gateway README, the changelog entry and the descriptor's wire-key comment all described the previous behaviour, and that comment had the split case backwards, saying the download exceeds the descriptor size when it is the size that exceeds the download. Quota eviction is dropped from the two comments naming it as a cause of metadata outliving its files, because eviction removes the whole bag directory rather than files inside it. --- docs/api/rest.rst | 16 +- src/ros2_medkit_gateway/CHANGELOG.rst | 2 +- src/ros2_medkit_gateway/README.md | 8 +- .../core/http/handlers/bulkdata_handlers.hpp | 51 ++++-- .../ros2_medkit_gateway/dto/bulkdata.hpp | 8 +- .../src/http/handlers/bulkdata_handlers.cpp | 61 ++++++- .../test/test_bulkdata_handlers.cpp | 150 +++++++++++++++++- 7 files changed, 254 insertions(+), 42 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 68182a22e..5201645f8 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1804,12 +1804,16 @@ and the API reports the recording's total instead. in several storage files rather than one. Three things are then true at once, and they are meant to be read together: -- the download hands over the **first** storage file of the recording, the one - its own metadata names first, which is where the recording starts. Which file - that is does not depend on the host's filesystem and does not change between - two requests for the same recording, -- ``x-medkit.storage_files`` says **how many** storage files there are, so a - client can tell it received a part and know how many parts the recording has, +- the download hands over the **first storage file the recording's own metadata + names that is on disk**. Normally that is the first segment, where the + recording starts. A named segment that is missing from disk is skipped in + favour of the next one that is there, and when none of them is on disk the + download fails rather than serving some other file that happens to sit in the + bag directory. Which file is served does not depend on the host's filesystem + and does not change between two requests for the same recording, +- ``x-medkit.storage_files`` says **how many storage files the metadata names**, + so a client can tell it received a part and know how many parts the recording + was recorded in, - ``size`` stays the **whole recording**, as does the nested ``environment_data.snapshots[].size_bytes``, while the download's ``Content-Length`` is that one file, so ``size`` exceeds ``Content-Length``. diff --git a/src/ros2_medkit_gateway/CHANGELOG.rst b/src/ros2_medkit_gateway/CHANGELOG.rst index 20e546ba9..df1ab3e61 100644 --- a/src/ros2_medkit_gateway/CHANGELOG.rst +++ b/src/ros2_medkit_gateway/CHANGELOG.rst @@ -4,7 +4,7 @@ Changelog for package ros2_medkit_gateway Forthcoming ----------- -* Downloading a rosbag recording that is split across several storage files, past ``snapshots.rosbag.max_bag_size_mb``, now hands over the first of them - the one the recording's own ``metadata.yaml`` names first, which is where the recording starts - instead of whichever ``.db3`` or ``.mcap`` the gateway host's directory listing happened to yield first. Which segment a client received was previously decided by the filesystem and could differ between two requests for the same recording. The rosbag descriptor gains ``x-medkit.storage_files``, the number of storage files the recording holds, so a client can tell a whole recording from one part of a split one. It is omitted when the gateway cannot read the recording's metadata, so an absent field means "not known here" rather than "one". The descriptor ``size`` is unchanged and still reports the whole recording for a split, which is why it exceeds the download's ``Content-Length``. +* Downloading a rosbag recording that is split across several storage files, past ``snapshots.rosbag.max_bag_size_mb``, now hands over the first storage file the recording's own ``metadata.yaml`` names that is on disk, normally the first segment, where the recording starts. A named segment missing from disk is skipped in favour of the next one that is there, and when none of them is on disk the download fails rather than serving another file. Previously the gateway walked the bag directory and served whichever ``.db3`` or ``.mcap`` it yielded first, so which segment a client received was decided by the host's filesystem and could differ between two requests for the same recording, and a file the recording never named could be served under its id. A bag directory is now read only when the recording will not say what it holds, meaning no ``metadata.yaml``, one that cannot be read or parsed, or a ``relative_file_paths`` naming nothing. The rosbag descriptor gains ``x-medkit.storage_files``, how many storage files the recording's metadata names, so a client can tell a whole recording from one part of a split one. It is omitted in exactly those cases where the metadata does not answer, so an absent field means "not known here" rather than "one". The descriptor ``size`` is unchanged and still reports the whole recording for a split, which is why it exceeds the download's ``Content-Length``. * The ``size`` a rosbag descriptor reports under ``GET /{entity}/bulk-data/rosbags`` is the storage file the download serves, measured through the same resolver the download uses, instead of the bag directory's total. The two differ by ``metadata.yaml``, which is never served, so every listing used to overstate its own download, 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. A bag whose ``metadata.yaml`` names exactly one storage file now resolves to that file rather than to whichever ``.db3`` or ``.mcap`` the directory yielded first, so a stray file beside the recording can no longer be served and sized in place of the real one, and the gateway and the fault manager decide it from the same field. A recording split across several storage files keeps the whole recording's figure, which is what the fault manager reports for it. A bag directory the gateway cannot read now costs its own row rather than the whole request: the resolver used the throwing filesystem calls, so one unreadable directory, or one removed by quota eviction mid-walk, answered ``500`` and took every other recording of that entity out of the listing with it. The gateway README no longer documents ``GET /faults/{code}/snapshots`` and ``.../snapshots/bag``, which have answered ``404`` since ``0.2.0``, because snapshots are returned inline with the fault and recordings are downloaded through the bulk-data endpoints. 0.7.0 (2026-08-27) diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index c62f8c21c..5062ef3ec 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -1165,9 +1165,11 @@ download serves one storage file, verbatim, named `.` 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 then the download hands over the first of -them, `x-medkit.storage_files` says how many there are, and `size` stays the -whole recording, so it exceeds the download's `Content-Length`. See +across several storage files, and then the download hands over the first one the +recording's own metadata names that is on disk (a named file missing from disk is +skipped, and if none is there the download fails rather than serving another +file), `x-medkit.storage_files` says how many the metadata names, and `size` +stays the whole recording, so it exceeds the download's `Content-Length`. See [the size rule](../../docs/api/rest.rst) in the REST API reference for the full statement. 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 d1d00f2a0..482e95b88 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 @@ -127,16 +127,29 @@ class BulkDataHandlers { * transfer is partial, alongside ``x-medkit.storage_files`` in the descriptor. * See the size rule in ``docs/api/rest.rst``. * - * The bag's own ``metadata.yaml`` decides: the first file it names in - * ``relative_file_paths`` that is on disk is the answer. That order is the - * capture order, so for a split recording the first name is where the - * recording begins and a client that fetches one gets its start rather than an - * arbitrary slice. A name that is not on disk is skipped, because quota - * eviction and a half-copied bag both leave metadata naming a file that is - * gone and the route would answer 500 on a recording whose other segments are - * readable. Only when the bag will not say - no metadata, unreadable metadata, - * or not one named file present - is the directory scanned for the first - * ``.db3`` or ``.mcap`` in whatever order it yields. + * The bag's own ``metadata.yaml`` decides, and when it has decided nothing + * else gets a vote. The answer is the first file named in + * ``relative_file_paths`` that is on disk. That order is the capture order, so + * for a split recording the first name is where the recording begins and a + * client that fetches one gets its start rather than an arbitrary slice. A + * name that is not on disk is skipped, because a half-copied bag leaves + * metadata naming a file that is gone and failing the request there would cost + * a recording whose other segments are readable. A name that is absolute or + * climbs through ``..`` is skipped as well: it would resolve outside the bag + * directory and put a file that is not part of the recording on the wire. + * + * When the bag named files and none of them is on disk the answer is the empty + * string. Falling through to the directory there served whatever ``.db3`` or + * ``.mcap`` sat beside the recording, under this recording's id and against a + * ``storage_files`` count the served file is not a member of. + * + * Only when the bag will not say what it holds - no ``metadata.yaml``, one + * this process cannot read or parse, or a ``relative_file_paths`` naming + * nothing - is the directory scanned for the first ``.db3`` or ``.mcap`` in + * whatever order it yields. That is the same set of shapes + * ``detail::rosbag_storage_file_count`` declines, which is the invariant: the + * count and the served file are both read from the metadata, or both from the + * directory, never one from each. * * @param path Path to rosbag (can be file or directory) * @return Resolved file path, or empty string if not found @@ -251,11 +264,19 @@ bool rosbag_resolved_by_fault_code(const nlohmann::json & rosbag_data, const std * reason, and a client holding one segment has no way to learn that the rest of * the recording exists. A ``1`` says the transfer was the whole recording. * - * nullopt when the bag will not say - no metadata, unreadable metadata, or not - * the shape rosbag2 writes - and the field is then omitted from the descriptor - * rather than defaulted. Counting the directory's ``.db3`` / ``.mcap`` files - * instead would count a stray beside the recording, and defaulting to one would - * claim a recording is whole on the evidence of nothing. + * nullopt when the bag will not say - no metadata, unreadable metadata, not the + * shape rosbag2 writes, or a ``relative_file_paths`` naming nothing - and the + * field is then omitted from the descriptor rather than defaulted. Counting the + * directory's ``.db3`` / ``.mcap`` files instead would count a stray beside the + * recording, and defaulting to one would claim a recording is whole on the + * evidence of nothing. A list naming nothing is declined rather than reported as + * zero for the same reason: the recording is not empty, the bag did not answer. + * + * The set of shapes declined here is exactly the set on which + * ``BulkDataHandlers::resolve_rosbag_file_path`` falls back to the directory, so + * the count and the served file are read from the metadata together or from the + * directory together. A count taken from one source describing a file chosen by + * the other is the state this pairing exists to make unreachable. * * Never throws, for the same reason as the two helpers around it: it runs once * per row of a listing, and one unreadable recording must not cost the entity's 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 b94ea81ed..679b650dd 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 @@ -58,15 +58,15 @@ inline constexpr std::string_view dto_name = "BulkDataCate // size - byte count the download route serves for this item // (required). For a rosbag held in one storage file that is // the file, not the bag directory's total. For one split -// across several it is the whole recording, which the -// download exceeds by handing over the first file alone +// across several it is the whole recording, so it exceeds +// what the download sends, which is one storage file // creation_date - ISO 8601 timestamp string (required) // description - optional human-readable description // x-medkit - optional open vendor extension object. For rosbags: // {fault_codes, duration_sec, format, recording_id, // storage_files}, the last of which is how many storage -// files the recording holds and is omitted when the bag's -// metadata cannot be read. For user uploads: an arbitrary +// files the recording's metadata names and is omitted when +// that metadata cannot be read. For user uploads: an arbitrary // metadata JSON object set by the uploader. // ============================================================================= struct BulkDataDescriptor { 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 c51ae7a0f..05e5473f8 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -112,6 +112,30 @@ std::optional> rosbag_relative_file_paths(const std::st } } +/// Does @p name, joined onto the bag directory, stay inside it? +/// +/// The names come out of a ``metadata.yaml`` on disk and are joined onto the bag +/// path, so they are input rather than a constant. An absolute name replaces the +/// bag path outright, because that is what ``operator/`` does with an absolute +/// right-hand side, and a name climbing through ``..`` walks out of the +/// directory. Either way the resolver would return a path outside the recording +/// and the download would stream that file under the recording's id. +/// +/// rosbag2 writes plain basenames here, so nothing legitimate is refused. This +/// is about what a bag directory that is not what it claims can name. +bool rosbag_name_stays_in_bag(const std::string & name) { + const std::filesystem::path candidate(name); + if (candidate.is_absolute() || candidate.has_root_name()) { + return false; + } + for (const auto & part : candidate) { + if (part == "..") { + return false; + } + } + return true; +} + } // namespace BulkDataHandlers::BulkDataHandlers(HandlerContext & ctx) : ctx_(ctx) { @@ -176,19 +200,36 @@ std::string BulkDataHandlers::resolve_rosbag_file_path(const std::string & path) // ``x-medkit.storage_files`` in the descriptor is what tells that client the // rest of the recording exists (see rosbag_storage_file_count). // - // A name that is not on disk is skipped rather than returned: quota eviction - // and a half-copied bag both leave metadata naming a file that is gone, and - // resolving to it would answer 500 for a recording whose other segments are - // readable. When the bag says nothing at all - no metadata, unreadable - // metadata, or not one named file present - the walk below is the fallback. - if (const auto names = rosbag_relative_file_paths(path); names) { + // A name that is not on disk is skipped rather than returned: a half-copied + // bag leaves metadata naming a file that is gone, and resolving to it would + // fail the request for a recording whose other segments are readable. + // + // When the bag HAS named files and none of them is on disk, the answer is + // nothing. The directory is deliberately not consulted then: it is the same + // question the bag already answered, and reaching past that answer served + // whatever .db3 or .mcap happened to sit beside the recording - a stray, a + // copy, a segment of a different bag - under this recording's id, and counted + // in ``x-medkit.storage_files`` a list the served file is not a member of. A + // client received neither the recording nor an error, and nothing in the + // response said which. Empty is honest: the listing keeps the row's own figure + // and the download answers its own error. + // + // The walk below is for a bag that will not say what it holds at all: no + // metadata, metadata this process cannot read or parse, or a list naming + // nothing (see rosbag_storage_file_count, which declines the same shapes). + // Metadata decides both the count and the served file, or neither does. + if (const auto names = rosbag_relative_file_paths(path); names && !names->empty()) { for (const auto & name : *names) { + if (!rosbag_name_stays_in_bag(name)) { + continue; + } const std::filesystem::path named = std::filesystem::path(path) / name; std::error_code named_ec; if (std::filesystem::is_regular_file(named, named_ec) && !named_ec) { return named.string(); } } + return ""; } std::filesystem::directory_iterator it(path, ec); @@ -244,8 +285,14 @@ std::optional rosbag_storage_file_count(const std::string & bag_pat // walk would count strays beside the recording, and a bag this process cannot // read would count zero, which reads as an empty recording rather than as an // unread one. + // + // A list naming nothing is that same zero by another route, so it is declined + // here rather than reported: the recording is not empty, the bag simply did + // not say what it holds. The resolver treats that shape identically and falls + // back to the directory, which keeps the invariant that the count and the + // served file come from the metadata together or from the directory together. const auto names = rosbag_relative_file_paths(bag_path); - if (!names) { + if (!names || names->empty()) { return std::nullopt; } return names->size(); diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index 190561d03..a72e799d7 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -285,15 +285,25 @@ class RosbagBagDirectoryTest : public ::testing::Test { /// 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. + /// + /// An empty @p storage_files is written as the flow-style ``[]``, which is an + /// empty sequence. A block sequence with no items under it is a YAML *null* + /// instead, and a null is refused one step earlier than an empty list is, by + /// the shape check rather than by the count, so a test built on one passes + /// without ever reaching the rule it names. 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"; + " message_count: 0\n"; + if (storage_files.empty()) { + yaml += " relative_file_paths: []\n"; + } else { + yaml += " relative_file_paths:\n"; + for (const auto & file : storage_files) { + yaml += " - " + file + "\n"; + } } yaml += " ros_distro: jazzy\n"; write_file(dir / "metadata.yaml", yaml); @@ -602,8 +612,8 @@ TEST_F(RosbagBagDirectoryTest, ASplitRecordingSkipsAFirstSegmentThatIsNoLongerOn TEST_F(RosbagBagDirectoryTest, ASplitRecordingWithNoSegmentLeftResolvesToNothing) { // Nothing to hand over, and the download route turns an empty resolution into - // its 500. The count still answers, because the metadata is readable and it is - // the recording's own record of what it held. + // its scoped error. The count still answers, because the metadata is readable + // and it is the recording's own record of what it held. const auto split_dir = bag_dir_ / "split_all_gone"; std::filesystem::create_directories(split_dir); write_metadata(split_dir, {"recording_0.db3", "recording_1.db3"}); @@ -624,6 +634,129 @@ TEST_F(RosbagBagDirectoryTest, ASplitRecordingWithNoSegmentLeftResolvesToNothing EXPECT_EQ(descriptors[0].size, 4242u) << "an unmeasurable recording keeps the figure its row carried"; } +// === A readable metadata is the whole answer, the directory is not consulted === +// The directory walk exists for a bag that will not say what it holds. Reaching +// it after the bag HAS said, because none of the names it gave is on disk, put a +// file the recording never named on the wire under that recording's id and under +// a storage_files the served file is not one of. What a client received was then +// neither the recording nor an error, and nothing in the response said so. + +TEST_F(RosbagBagDirectoryTest, ASplitRecordingWithNoSegmentLeftDoesNotServeAStrayBesideIt) { + // Several names, none on disk, a stray storage file beside them. The stray is + // a .db3, so the directory walk would reach it, and it is not one of the two + // files the metadata names. + const auto split_dir = bag_dir_ / "split_gone_with_stray"; + std::filesystem::create_directories(split_dir); + write_file(split_dir / "stray.db3", std::string(8192, 's')); + write_metadata(split_dir, {"recording_0.db3", "recording_1.db3"}); + ASSERT_FALSE(std::filesystem::exists(split_dir / "recording_0.db3")); + ASSERT_FALSE(std::filesystem::exists(split_dir / "recording_1.db3")); + ASSERT_EQ(first_in_directory_order(split_dir), split_dir / "stray.db3") + << "the directory walk cannot reach the stray, so nothing is being told apart"; + + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(split_dir.string()), ""); }) + << "a file the recording never named must not be served in its place"; + + const json row{{"fault_code", "SPLIT_FAULT"}, + {"recording_id", "fault_SPLIT_FAULT_1738664999004"}, + {"file_path", split_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", 4242}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + ASSERT_TRUE(descriptors[0].x_medkit->contains("storage_files")); + EXPECT_EQ((*descriptors[0].x_medkit)["storage_files"], 2) << "the count is still what the metadata names"; + EXPECT_EQ(descriptors[0].size, 4242u) << "and the row's figure is kept, not the stray's size"; +} + +TEST_F(RosbagBagDirectoryTest, AWholeRecordingWhoseOnlyFileIsGoneDoesNotServeAStrayBesideIt) { + // One name, not on disk, a stray storage file beside it. The same rule as the + // several-name case above, on the shape that reaches a user first, because a + // recording held in one file is the normal case. + const auto gone_dir = bag_dir_ / "single_gone_with_stray"; + std::filesystem::create_directories(gone_dir); + write_file(gone_dir / "stray.db3", std::string(8192, 's')); + write_metadata(gone_dir, {"recording_0.db3"}); + ASSERT_FALSE(std::filesystem::exists(gone_dir / "recording_0.db3")); + ASSERT_EQ(first_in_directory_order(gone_dir), gone_dir / "stray.db3"); + + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(gone_dir.string()), ""); }) + << "a file the recording never named must not be served in its place"; + + const json row{{"fault_code", "GONE_FAULT"}, + {"recording_id", "single_gone_with_stray"}, + {"file_path", gone_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", 777}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + ASSERT_TRUE(descriptors[0].x_medkit->contains("storage_files")); + EXPECT_EQ((*descriptors[0].x_medkit)["storage_files"], 1); + EXPECT_EQ(descriptors[0].size, 777u) << "the listing keeps the row's figure rather than measuring the stray"; +} + +TEST_F(RosbagBagDirectoryTest, AnEmptyStorageFileListIsTreatedAsABagThatWillNotSay) { + // A metadata that names nothing (`relative_file_paths: []`). That is a bag + // which did not answer, not a bag holding zero files: there is a storage file + // in the directory. Counting it at zero would describe the recording as empty, + // which is the value the count exists to avoid, so the field is omitted and the + // directory walk answers, exactly as for metadata that cannot be read at all. + const auto empty_list_dir = bag_dir_ / "empty_list"; + std::filesystem::create_directories(empty_list_dir); + write_file(empty_list_dir / "recording_0.db3", std::string(3072, 'e')); + write_metadata(empty_list_dir, {}); + + EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(empty_list_dir.string()), + (empty_list_dir / "recording_0.db3").string()) + << "an empty list is not an answer, so the directory is still consulted"; + EXPECT_FALSE(handlers::detail::rosbag_storage_file_count(empty_list_dir.string()).has_value()) + << "a bag that named nothing must not be counted at zero"; + + const json row{{"fault_code", "EMPTY_LIST"}, + {"recording_id", "empty_list"}, + {"file_path", empty_list_dir.string()}, + {"format", "sqlite3"}, + {"size_bytes", 555}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors({row}, {}); + ASSERT_EQ(descriptors.size(), 1u); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + EXPECT_FALSE(descriptors[0].x_medkit->contains("storage_files")) << "omitted, not zero"; +} + +TEST_F(RosbagBagDirectoryTest, ANameThatLeavesTheBagDirectoryIsSkipped) { + // A name that climbs out of the bag directory, and a name that is absolute. + // The names come out of a file on disk and are joined onto the bag path, so a + // name that climbs out of the directory, or replaces it outright by being + // absolute, would resolve to a file outside the recording and the download + // would stream it. Both are skipped, and with no other named file present that + // leaves nothing to serve. + const auto escape_dir = bag_dir_ / "escape"; + std::filesystem::create_directories(escape_dir); + const auto outside = bag_dir_ / "outside_target.db3"; + write_file(outside, std::string(6144, 'o')); + ASSERT_TRUE(std::filesystem::exists(outside)) << "the escape target has to exist, or nothing is being told apart"; + + write_metadata(escape_dir, {"../outside_target.db3"}); + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(escape_dir.string()), ""); }) + << "a relative name that climbs out of the bag must not be served"; + + write_metadata(escape_dir, {outside.string()}); + EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(escape_dir.string()), ""); }) + << "an absolute name replaces the bag path outright and must not be served"; + + // Positive control on the same harness: the same file, named the way rosbag2 + // names one, is served. So the two refusals above are the escape being + // refused and not the resolver failing to find anything at all. + write_file(escape_dir / "recording_0.db3", std::string(1024, 'r')); + write_metadata(escape_dir, {"recording_0.db3"}); + EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(escape_dir.string()), (escape_dir / "recording_0.db3").string()); +} + TEST_F(RosbagBagDirectoryTest, StorageFileCountIsOneForAWholeRecordingAndAbsentWhenTheBagWillNotSay) { // One has to be stated rather than left out, because an absent field already // means something else here: that this side could not read the recording's @@ -815,6 +948,10 @@ TEST_F(UnreadableBagTest, AnUnreadableBagCostsItsOwnRowAndNotTheListing) { // 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()), ""); }); + // The count reaches the filesystem on its own, before any metadata is read, to + // decide whether the path is a bare storage file. It runs on the same row of + // the same listing, so it has to decline an unreadable bag the same way. + EXPECT_NO_THROW({ EXPECT_FALSE(handlers::detail::rosbag_storage_file_count(loop_.string()).has_value()); }); } // The second shape: a directory whose mode denies everyone. This one does test @@ -842,6 +979,7 @@ TEST_F(UnreadableBagTest, AModeZeroDirectoryIsAlsoDeclinedRatherThanThrown) { EXPECT_NO_THROW({ EXPECT_FALSE(handlers::detail::rosbag_served_bytes(locked.string()).has_value()); }); EXPECT_NO_THROW({ EXPECT_EQ(BulkDataHandlers::resolve_rosbag_file_path(locked.string()), ""); }); + EXPECT_NO_THROW({ EXPECT_FALSE(handlers::detail::rosbag_storage_file_count(locked.string()).has_value()); }); std::filesystem::permissions(locked, std::filesystem::perms::owner_all, ec); }