Conversation
mfaferek93
reviewed
Sep 14, 2026
bburda
force-pushed
the
fix/bulk-data-size-and-stale-snapshot-routes
branch
2 times, most recently
from
September 20, 2026 14:02
cccf127 to
df614a6
Compare
A rosbag2 recording is a directory with one storage file and
metadata.yaml. The download route resolves the storage file and streams
only that file. The listing reported the figure the fault manager
stores, which is the size of the whole directory. The two numbers answer
different questions. The stored figure is the footprint of the
recording against its disk quota. The listed figure is what a client is
about to fetch. So every listing overstated the download, on a short
recording by about a tenth of the transfer. A client that sized a
buffer or a progress bar from the listing never reached the end.
The listing now measures the file that the download resolves, through
the same resolver. The promised length is the length that arrives.
resolve_rosbag_file_path becomes a public static member so the listing
can call it. When this process cannot see the bag, the descriptor keeps
the stored figure. It is the only number available, and a zero would
describe the recording as empty. The stored figure itself is unchanged,
so quota accounting still counts the bytes the recording occupies.
rest.rst and the DTO comment state that size is the byte count the
download serves.
Also drop the removed snapshot endpoints from the gateway README and
from the quick start of the snapshots tutorial. GET
/faults/{code}/snapshots and .../snapshots/bag answer 404. A fault
returns its snapshots inline, and recordings are downloaded through the
bulk-data endpoints. The migration table in the tutorial stays, because
it points a reader at the replacements.
A recording is stored as a directory and served as a single file, so it has two sizes. The row carried one number for both jobs: the directory total. That total is what the recording costs against max_total_storage_mb. Every API answer that quoted it overstated the download by metadata.yaml, on a short recording by about a tenth of the transfer. The environment_data.snapshots[] entry next to a download link was the worst case, because a client sizes its transfer from that number. rosbag_served_bytes() now answers the reporting question on its own. It measures the storage file that the bag's metadata.yaml names. It reads the metadata through rosbag2_storage::MetadataIo, the library that wrote it. GetFault, GetSnapshots, GetRosbag and ListRosbags report their size through it. RosbagFileInfo::size_bytes and the quota are unchanged. They still count the whole directory, which is what eviction frees. When no single served file can be named, the function returns the stored total. This covers a missing metadata.yaml, one that cannot be parsed, a named file that is gone, and a recording split across several storage files past the maximum bag size. None of these is an error. The stored total is a real measurement of the recording. A zero would describe it as empty. rest.rst now states the rule once. The descriptor size, the nested size_bytes and the Content-Length of the download are the same number, and that number is the size of the storage file. A split recording is the exception and reports its total.
… payload
The GetSnapshots response carried rosbag.download_url, built as
/api/v1/faults/{code}/snapshots/bag. That route was removed in 0.2.0, so
the field gave every caller a URL that answers 404.
No code in this repository reads the field. The gateway builds its own
entity-scoped bulk-data URI from the recording id it receives on the
GetFault snapshot entries.
The field is removed, and this payload gets no new URL. A recording is
addressed under its entity, as
/api/v1/{entity-type}/{id}/bulk-data/rosbags/{recording_id}. Which of
the four entity types owns a given source is part of the gateway's
discovery model. The fault manager holds the recording id and the
reporting source, but it does not hold that mapping. A URL built here
would be a guess at one of four prefixes, and a wrong URL that looks
right misleads the caller.
Service-level tests now cover the size that GetSnapshots, GetRosbag and
ListRosbags report, next to the existing GetFault test. A shared bag
fixture backs them. The test helpers call file_size() without a cast.
It already returns uintmax_t, so the cast was an identity cast that
-Wuseless-cast flags.
…unreadable bag Sizing rule. For a bag directory, the listing measures a file only when metadata.yaml names exactly one storage file, the name is a direct child of the bag with a .db3 or .mcap extension, and the file is present. In every other case the descriptor keeps the row's figure, which is the fault manager's answer for the same recording. That covers a split recording, a missing or unparsable metadata.yaml, a name that escapes the directory, and a named file that is gone. The fault manager decides "split" from the same relative_file_paths field, so both API surfaces report one size for one recording. A path that is itself a storage file is one storage file, and the listing sizes it as it is. A split recording keeps its total because the download hands over one segment, and no single file describes the transfer. The descriptor size is then larger than the Content-Length of the download. rest.rst states this as the way a client recognises a partial download of a split recording. The Content-Length entry links to that rule with explicit link text, because the label sits on a paragraph. Resolver. When the metadata names one storage file under the same rule and the file exists, the resolver returns that file. Otherwise it scans the directory for the first .db3 or .mcap, so the download always has a file to hand over when the directory holds one. A stray file next to the recording, such as a leftover segment or a copy, is never sized as the recording. The gateway reads relative_file_paths with yaml-cpp, which it already links. It does not link rosbag2_storage. Filesystem errors. Every filesystem call in the resolver and in the size helper uses the std::error_code overload. EACCES on one bag directory, or ENOENT when quota eviction removes a bag during the walk, now leaves that one recording unmeasured. The listing handler has no catch in its chain, so the exception answered 500 and dropped every other recording of the entity. The download route shares the resolver and gets the same protection. The comments on the resolver and on rosbag_served_bytes state when the listing and the download report the same length. README. The download is one storage file streamed verbatim, named <recording_id>.<format> (.mcap or .sqlite3). It is not an archive and it does not include metadata.yaml. The playback section runs ros2 bag info and ros2 bag play on the downloaded file, with no unpacking step and no --storage flag, for both storage formats. The Postman collection drops its three requests to the snapshot routes that were removed in 0.2.0. The collection has no bulk-data section to move them to. Tests. A new integration test compares a descriptor's size with the bytes its own download delivers and with Content-Length. Unit tests cover a split recording, an unreadable directory, a bare storage file, a stray file, a named file that is gone, a name that climbs out of the bag, an absolute name, a name in a subdirectory and a name without a storage extension. The stray-file test names the storage file that the directory scan would not reach first. The mode-0000 test drops CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH from its own thread's effective set when it runs as root. It restores them through a guard object, and it asserts that the permission is denied before it asserts anything about the resolver. A test uses the result of a nodiscard is_directory call. A fault manager test pins the same 4096-byte answer for a named recording with a stray file beside it.
…side it rosbag_served_bytes sizes the named file only when the name is a direct child of the bag directory with a .db3 or .mcap extension; any other name falls back to the stored total, the same rule the gateway applies. The stray-file test's cross-reference names that rule.
rest.rst now gives the full condition. The descriptor size, size_bytes and Content-Length are one number when metadata.yaml names exactly one storage file, the name is a direct child of the bag with a .db3 or .mcap extension, and that file is present. The page lists the four cases where the reported size is the stored total while the download hands over one file. It states that a bag directory with no storage file answers 500. It also corrects the on-disk extension of a sqlite3 recording from .sqlite3 to .db3.
bburda
force-pushed
the
fix/bulk-data-size-and-stale-snapshot-routes
branch
from
September 20, 2026 18:47
df614a6 to
0fc66e3
Compare
mfaferek93
reviewed
Sep 28, 2026
| bag = bag.parent_path(); // tolerate a trailing slash | ||
| } | ||
| const std::filesystem::path named = (bag / relative_name).lexically_normal(); | ||
| if (named.parent_path() != bag) { |
Collaborator
There was a problem hiding this comment.
The containment here is lexical only: a symlink recording_0.db3 -> <any file the gateway can read> inside the bag passes both tests, is_regular_file at :207 follows it, and the download serves the link target at its size, which is exactly the escape this function is meant to close (the scan at :219-225 follows links the same way, and the writer of the bag directory is the same actor the comment above names). Decline anything whose symlink_status(named).type() is not file_type::regular here and in the scan, and mirror it in named_storage_file in rosbag_capture.cpp:1282, otherwise the fault manager keeps sizing the target while the listing falls back and the two sides part again.
| // Include rosbag info if available | ||
| // Include rosbag info if available. | ||
| // | ||
| // No download URL. This payload used to carry one built as |
Collaborator
There was a problem hiding this comment.
The PR text says this breaking change is recorded in the fault manager changelog, but src/ros2_medkit_fault_manager/CHANGELOG.rst is untouched in this diff and has no Forthcoming section. Add the entry (dropped rosbag.download_url here, and size_bytes on GetFault/GetSnapshots/GetRosbag/ListRosbags now being the storage file rather than the directory total), the way #620 was recorded under Forthcoming before 0.7.0 was cut.
| present - the normal case - and that number is the storage file. A recording also | ||
| has a footprint on the gateway host, which is larger because the directory holds | ||
| ``metadata.yaml`` as well. That figure is what the recording spends against its | ||
| storage quota and is not reported by the API. |
Collaborator
There was a problem hiding this comment.
This sentence says the footprint is never reported by the API, and the paragraph twelve lines below says that in the four fallback shapes size and the nested size_bytes carry the recording's stored total, which is that footprint. Narrow it to the single-file case ("is not reported for such a recording") so the rule does not contradict itself.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A rosbag2 recording is a directory with one storage file plus
metadata.yaml. The bulk-data download serves the storage file alone, but every size in the API was the fault manager's stored figure, the whole directory. Each listing overstated its own download bymetadata.yaml.Now:
sizeunder/bulk-data/rosbagsis measured on the file the bag'smetadata.yamlnames, when the download resolves that same file.GetFault,GetSnapshots,GetRosbagandListRosbagsis the storage file named by the bag'smetadata.yaml. The storedsize_bytesand themax_total_storage_mbquota still count the directory, which is what eviction frees.max_bag_size_mb) the reported size falls back to the stored total, never to zero. Both layers decide this from the samerelative_file_pathsfield and both follow a name only to a direct child of the bag directory with a.db3or.mcapextension. In those cases the download hands over one file from the directory and the descriptor size can exceed the transfer, which is the client's signal; a directory with no storage file at all answers 500 on download. Which file the download serves for a split is unchanged, see bulk-data rosbags: a recording split across several storage files downloads as one segment, chosen by directory order #675.metadata.yamlnames exactly one present file inside the bag, the listing and the download resolve to that file. When the named file is absent, the download still hands over what the directory holds, and the listing keeps the stored figure for that row. A bare storage-file path is sized as that file.rest.rststates the rule once: descriptorsize, nestedsize_bytesandContent-Lengthare the same number for a recording whose metadata names its single present storage file, and lists the cases where they part.Stale leftovers removed: the gateway README and the tutorial quick start documented
GET /faults/{code}/snapshotsand.../snapshots/bag, both 404 since the endpoints were removed. The README playback block told the reader totar -xzfthe download. It now showsros2 bag infoandros2 bag playon the file the download names, run on both storage formats. The postman collection drops its three requests at the removed routes. TheGetSnapshotsresponse no longer carriesrosbag.download_url, which pointed at that removed route. Nothing in the repository reads it and no gateway route forwards that payload.Issue
Type
Breaking: the
GetSnapshotsservice response no longer carriesrosbag.download_url. Recorded in the fault manager changelog. The REST API shape is unchanged.Testing
Gateway,
test_bulkdata_handlers.cpp: descriptor size equals the served file and not the directory total; a bag the process cannot see keeps the stored figure; a two-segment recording is listed at the stored total; a stray second file beside a single named one is sized by the named file, with the test naming whichever file directory order would not reach first; a named file that is gone is not sized by whatever is left beside it; a bare storage-file path; names that climb out of the bag, are absolute, sit in a subdirectory or lack a storage extension are not followed, and one such bag that also holds a real storage file still serves that file; a listing with a symlink-loop bag answers with every row; a mode-0000 bag is declined without a throw, with the test droppingCAP_DAC_OVERRIDEfrom its own thread when it runs as root so the permission is denied in CI too. Each test was made to fail first.Fault manager,
test_rosbag_capture.cppandtest_fault_manager.cpp: the helper on a well-formed bag, missing metadata, unparsable metadata, a named file that is gone, a name outside the bag (four shapes), a multi-file recording, real service calls through all four services, and aGetSnapshotspayload with nodownload_url. Quota tests untouched.Integration,
test_bulk_data_api.test.py: descriptor size equals the downloaded body length.Full gateway and fault manager package suites pass locally on jazzy. Lint and docs build clean.
Checklist