APP-17380 Export single sequence - #6355
Closed
Katie Peters (katiepeters) wants to merge 2 commits into
Closed
Conversation
viambot
added
safe to test
viambot
added
safe to test
Three independent fixes to commands that already exist, split out of the `data export sequence` work that surfaced them. --parallel=0 hung. The flag is a uint with no validation, so 0 was accepted; with zero workers the ID producer blocks forever on a send nobody reads and the command hangs instead of failing. `data export binary filter` and the binary `dataset export` both did this. Rejected at the flag now, via a Validator on all three --parallel flags, which urfave runs only on values the user actually set. downloadSequenceBinaryBlobs' ad-hoc `if parallel == 0` fallback goes with it, and the five literal 100s collapse into defaultParallelBinaryDownloads. A failing download hung too, by a different route: getMatchingBinaryIDs sent into the id channel bare, so when an action failed and the workers cancelled and returned, the producer blocked forever on a full buffer. It selects on ctx now. actionOnBinaryData took only an id, so the driver's cancellable context could not reach it: on first error the in-flight action ran to completion against whatever context its closure had captured. Two of the six call sites were passing context.Background() outright. performActionOnBinaryDataFromFilter takes a ctx and threads it through. Adds pluralize and uses it for the "%d files" style messages, which said "1 files" throughout. Both hangs have regression tests that time out if the fix is removed.
Katie Peters (katiepeters)
force-pushed
the
APP-17380
branch
from
August 18, 2026 14:12
98f9faa to
0add325
Compare
viambot
added
safe to test
cli tests are named for the function under test -- TestDownloadBinarySkipsExisting drives downloadBinary, TestDataQuerySQLAction drives dataQuerySQLAction, and so on for twelve straight tests in client_test.go. The two added here were the only ones named for a command they never reached. Rather than rename them down, TestDataExportBinaryAction now drives dataExportBinaryAction, so it also covers --part-id becoming the filter. The producer-deadlock case folds in as a second subtest and gets a more realistic trigger: the download itself fails via BinaryDataByIDs, instead of a stubbed action that always errored. binaryFilterFake grows `repeat` and `byIDsErr` for it.
viambot
added
safe to test
Member
Author
|
Should not have been opened from fork |
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.
No description provided.