Update error handling to prevent partial / failed downloads from being marked complete#263
Update error handling to prevent partial / failed downloads from being marked complete#263
Conversation
pkg/rsstorage/internal/chunks.go
Outdated
| go func() { | ||
| err = w.readChunks(ctx, address, chunkDir, info.NumChunks, info.Complete, info.FileSize, pW) | ||
| }() | ||
| if err != nil { |
There was a problem hiding this comment.
Oops, didn't mean to delete my comment above, but I see it was fixed. Another issue here- the err var happens right away and won't get any async info.
There was a problem hiding this comment.
Good catch. I was going to comment here, too.
Co-authored-by: Brian Deitte <bdeitte@gmail.com>
Co-authored-by: Brian Deitte <bdeitte@gmail.com>
jstruzik
left a comment
jstruzik
left a comment
There was a problem hiding this comment.
LGTM other than Brian's comments
pkg/rsstorage/internal/chunks.go
Outdated
| for i := uint64(0); i <= numChunks; i++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| bytesRead, err = w.retryingChunkRead(ctx, i, address, chunkDir, complete, writer) | ||
| if err != nil { | ||
| errs <- writer.CloseWithError(err) | ||
| return | ||
| } | ||
| totalBytesWritten += bytesRead | ||
| }() |
There was a problem hiding this comment.
This loop needs to be sequential. Each call to retryingChunkRead will copy the bytes for the chunk to the writer we pass to it. If we're using async goroutines, the file could be assembled in a random order.
pkg/rsstorage/internal/chunks.go
Outdated
| go func() { | ||
| err = w.readChunks(ctx, address, chunkDir, info.NumChunks, info.Complete, info.FileSize, pW) | ||
| }() | ||
| if err != nil { |
There was a problem hiding this comment.
Good catch. I was going to comment here, too.
There was a problem hiding this comment.
A few additional comments beyond what Jon already flagged on the readChunks parallelization, the ReadChunked goroutine error check, and the writeChunks defer panic.
Edit: Claude posted these last four for me. I thought they were worth calling out.
| // Return normal file info | ||
| return true, nil, stat.Size(), stat.ModTime(), nil | ||
| found = true | ||
| chunkInfo = &info |
There was a problem hiding this comment.
Nit: with named returns, err is already zero-valued (nil) by default. The explicit err = nil assignments here and on line 128 are redundant.
There was a problem hiding this comment.
I like setting named returned explicitly for clarity / readability. I'll remove them.
| defer func(path string) { | ||
| removeErr := os.RemoveAll(path) | ||
| if removeErr != nil { | ||
| c.Logf("unbale to remove temp directory: %s", removeErr.Error()) |
There was a problem hiding this comment.
Typo: "unbale" → "unable"
4 participants
About
This PR updates the logic in the
rsstoragepackage to better handle errors when performing chunked reads or writes. Certain errors were not being handled or propagated before and could result in packages being marked as complete when they were actually just partially downloaded or broken. This addresses https://github.com/rstudio/package-manager/issues/16438.This fix involved checking for errors during chunk reads and writes, and making sure the checks were used to signal to callers that the operation failed and the package isn't ready. Additionally, checks were added to verify the number of bytes read or written equals the file size and returning an error when they don't match. Instances of ignored errors were also updated to check for error conditions in the event that they were also causing unexpected behavior.