fix(gateway): download a split recording's first file, and say how many there are - #684
Draft
bburda wants to merge 3 commits into
Conversation
…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.
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