disk import: Prioritize returning upload errors #992
Merged
Conversation
This file contains 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
When sending chunks of the file to upload tasks, we return an error if the channel to that task is closed. This situation will happen if the upload task has panicked or already returned due to an error. However, we check for an error from the sending task first, and the `failed to send chunks` error will be surfaced to the user, rather than the true problem that occurred in the upload task. In the inverse situation, where the reader fails, the uploaders will return gracefully when their channels are closed, so we do not need to worry about a problem reading the file showing up as an upload error. Update our error handling to check for an error from the upload tasks, then the reader if those returned cleanly.
|
Issue reported by a customer in https://oxidecomputer.slack.com/archives/C089VG8LHPG/p1738198958814399 |
ahl
reviewed
Jan 30, 2025
The index of the failed task doesn't provide more context to users and might be accidentally construed as a backtrace. Just list the bare errors for each failed request. Also, use a standard `DiskImportError::context` if a single task failed.
If the file reader task fails to send a chunk to an upload task, we know this is because that task has either returned due to an error or panicked. In either case this isn't an error for the reader, which should return early as the overall request will fail.
The errors returned by a connection failure are long enough to wrap in a user's terminal. Add a `*` to the start to make distinguishing errors easier.
ahl
reviewed
Jan 31, 2025
ahl
approved these changes
Feb 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When sending chunks of the file to upload tasks, we return an error if the channel to that task is closed. This situation will happen if the upload task has panicked or already returned due to an error. However, we check for an error from the sending task first, and the
failed to send chunkserror will be surfaced to the user, rather than the true problem that occurred in the upload task.In the inverse situation, where the reader fails, the uploaders will return gracefully when their channels are closed, so we do not need to worry about a problem reading the file showing up as an upload error.
Update our error handling to check for an error from the upload tasks, then the reader if those returned cleanly.