fix: Handle accept result and download result errors #416
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
bgins
force-pushed
the
bgins/fix-accept-and-download-errors
branch
from
34eb0a9 to
f46c781
Compare
bgins
commented
Oct 24, 2024
| // we have already downloaded. Check the download path and download | ||
| // if results do not exist. | ||
| downloadPath := solver.GetDownloadsFilePath(dealContainer.ID) | ||
| if _, err := os.Stat(downloadPath); errors.Is(err, fs.ErrNotExist) { |
There was a problem hiding this comment.
Common advice online recommends using os.IsNotExist, but the docs for this function recommend that new code should use errors.Is(err, fs.ErrNotExist): https://pkg.go.dev/os#IsNotExist
If the path exists, we have already attempted to download the results.
bgins
force-pushed
the
bgins/fix-accept-and-download-errors
branch
from
f46c781 to
7124b34
Compare
The unbuffered error channel blocks on the receiver which does not get set up in time. The buffered channel will hold the message until the receiver is available.
2 tasks
walkah
approved these changes
Oct 31, 2024
| @@ -161,7 +164,7 @@ func (controller *JobCreatorController) subscribeToWeb3() error { | |||
| } | |||
|
|
|||
| func (controller *JobCreatorController) Start(ctx context.Context, cm *system.CleanupManager) chan error { | |||
| errorChan := make(chan error) | |||
| errorChan := make(chan error, 1) | |||
walkah
pushed a commit
that referenced
this pull request
Dec 10, 2024
* fix: Add accept result error handling * fix: Add download result error handling * fix: Check download path before downloading If the path exists, we have already attempted to download the results. * fix: Unblock error channel The unbuffered error channel blocks on the receiver which does not get set up in time. The buffered channel will hold the message until the receiver is available.
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.
Summary
This pull request makes the following changes:
acceptResulterrorsdownloadResulterrorsWe would like to handle these errors to avoid silently failing when running jobs from the CLI. Error reporting also supports observing this information in traces.
The unbuffered error channel blocks on the receiver which does not get set up before the send. The buffered channel will hold the message until the receiver is available to receive the message.
Task/Issue reference
Closes: #414
Test plan
Run a few jobs. Check that everything still works.
Running more than one job exercises the download check. If we attempt to download again, we would observe a
file existserror.To test the error cases, return a temporary error from the
downloadResultandacceptResultfunctions.Details (optional)
The new downloads check determines if we have already downloaded the results for a job. If the download path already exists, we skip the download. This check is necessary because our control loop checks for any completed deals and re-downloads the results. The solver may report completed deals for jobs whose results we already have.