fix: report a rosbag's served size, retire the removed snapshot routes - #679
Open
bburda wants to merge 8 commits into
Open
fix: report a rosbag's served size, retire the removed snapshot routes#679bburda wants to merge 8 commits into
bburda wants to merge 8 commits into
Conversation
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.
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.
… 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.
… 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.
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.
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.
7 tasks
std::filesystem::file_size and directory_entry::file_size already return uintmax_t, which is size_t on every platform this builds for, so the cast is identity and -Wuseless-cast flags it.
bburda
marked this pull request as ready for review
September 12, 2026 17:31
mfaferek93
reviewed
Sep 14, 2026
| // 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); |
Collaborator
There was a problem hiding this comment.
The gate above only checks that metadata names one file, not that it exists, so for a bag naming recording_0.db3 with only a stray recording_1.db3 on disk the resolver falls through to the directory scan and this row is sized by the stray, while the fault manager's rosbag_served_bytes returns the stored total for the same bag (its ANamedFileThatIsNotOnDiskFallsBackToTheStoredTotal builds exactly that directory). That breaks the one-recording-one-size rule rest.rst just added and the description's claim about the fallback. Return nullopt here unless resolved == bag_path / names->front().
| 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()); |
Collaborator
There was a problem hiding this comment.
If the metadata branch in resolve_rosbag_file_path (lines 171-177) were dropped, this test only fails when directory_iterator happens to yield recording_1.db3 before recording_0.db3, and that order is hash order on ext4 and APFS, so on a given runner the regression this test is named for can pass clean every time. Make it deterministic: walk the directory the way the fallback loop does and write metadata naming the file that walk would not return first, and replace the 4096u literal on 461 with named_size.
| 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. " |
Collaborator
There was a problem hiding this comment.
The description says the listing is exercised with a symlink-loop bag and a mode-0000 bag, but this test never calls fold_rosbag_rows_into_descriptors and skips under uid 0, which is what every CI job container runs as. The 0000 directory is the only case that reaches the resolver's directory_iterator(path, ec) EACCES branch, so the branch the resolver comment calls the 500 bug has no CI coverage. Either fix the description, or run this one under a non-root uid so it counts.
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 download resolves.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 "split" from the samerelative_file_paths. For a split the download hands over one file and the descriptor size exceeds the transfer, which is the client's signal. 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 file, the listing and the download resolve to that file, not to whatever the directory yields first. A bare storage-file path is sized as that file.rest.rststates the rule once: descriptorsize, nestedsize_bytesandContent-Lengthare the same number for a single-file recording.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 listed at the stored total, a stray second file beside a single named one sized by the named file, a bare storage-file path, and a listing with a symlink-loop bag and a mode-0000 bag that answers with every row. 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 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.Gateway suite 3845 tests, fault manager 732, 0 failures. Lint and docs build clean.
Checklist