fix(gateway): download a split recording's first file, and say how many there are - #684
Draft
bburda wants to merge 9 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.
…ny 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.
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.
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.
bburda
force-pushed
the
fix/bulk-data-size-and-stale-snapshot-routes
branch
from
September 18, 2026 18:04
6c05ddd to
cccf127
Compare
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
1 participant
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 recording split past
snapshots.rosbag.max_bag_size_mbdownloaded as whichever storage file the directory listing yielded first, and nothing in the descriptor said the recording had more.Now
resolve_rosbag_file_pathfollowsmetadata.yaml. When the metadata is readable and names files, the resolver returns the first named file that is on disk, normally the first segment, and returns nothing when none of them is on disk, rather than serving a file the recording never named. A name that is absolute or contains..is skipped. A recording without readable metadata, or with an empty file list, resolves by directory order as before. The listing and the download use the same resolver, so both name the same file. The rosbag descriptor'sx-medkitblock carriesstorage_files, the number of storage files the metadata names, omitted when the metadata cannot be read or names none. The descriptorsizefor a split stays the recording's total,Content-Lengthis the served file, andrest.rstsays so.Serving a split recording whole is not part of this.
The gateway changelog gets its first
Forthcomingsection, with this change and the descriptor-size change from #679, which had no gateway bullet.Stacked on #679.
Issue
Type
Testing
Tests in
test_bulkdata_handlers.cpp, each red before the change and red again under its injection: a three-segment recording resolves to the segment its metadata names first, which the test picks so that neither directory order nor lexical order can produce it; a first segment gone from disk is skipped for the next one the metadata names; a recording whose named segments are all gone resolves to nothing even with another file beside them; one missing name with a stray file beside it resolves to nothing; an empty file list falls back to directory order with no count; a name that escapes the bag directory is skipped;storage_filesis 1 for a whole recording and absent when the metadata cannot be read (with the count forced to 1 as the control); the download and the listing agree on the segment; the count helper does not throw on a symlink loop or an unreadable directory.Gateway suite 2899 tests, fault manager 3530, 0 failures. Lint, pre-commit and the docs build clean.
No integration test for a split recording: producing one needs 50 MB of data or a lower
max_bag_size_mbin the shared launch helper.Checklist