Clean up partially-downloaded ZIP files if aborted #388
Conversation
This fixes a bug where src-cli would leave partially-downloaded ZIP files behind when the user hit Ctrl-C while a download was ongoing. The `fetchRepositoryArchive` would then run into a context-cancelation error when doing the request or the `io.Copy` and not clean up the file. The code here checks whether the cause of the error is a cancelation and, if so, cleans up the zip file.
There was a problem hiding this comment.
LGTM. However, this is relying on your program not being sigkilled so cleanup can run. To make downloading more robust I'd recommend downloading to filepath.Join(dst, ".tmp") (or something) and renaming once download is complete. This technically still won't be "safe", since you haven't fflushed, but that is probably fine. You can use something like the renameio pkg to handle doing this safely.
I think I agree with this too, with the caveat that I'd still keep the logic to clean up wherever possible, except applied to that temp file.
Unfortunately, renameio doesn't work on Windows. shakes fist |
|
Regarding download-to-temp-file and rename: I don't think this is necessary, since I just changed the code to delete the file in case of any error and nothing else will access the file while that method is running. It doesn't need to be an atomic operation. I'm going to go ahead and merge this, but if someone can tell me what I'm missing and why this needs to be an atomic download-and-swap I'll try to change it. |
I think the concern here is the one Keegan called out: if |
|
Ah, thanks! Now I get it. I'll think about that a bit more, because we also don't want to leave tmp files lying around and probably want to clean those up when starting. Implementing that could go hand-in-hand with my thoughts about making the archive-caching a bit more efficient (right now, if turned on, we'd cache all archives, which means you can accumulate a large number if your default branch updates often) |
* Clean up partially-downloaded ZIP files if aborted This fixes a bug where src-cli would leave partially-downloaded ZIP files behind when the user hit Ctrl-C while a download was ongoing. The `fetchRepositoryArchive` would then run into a context-cancelation error when doing the request or the `io.Copy` and not clean up the file. The code here checks whether the cause of the error is a cancelation and, if so, cleans up the zip file. * Add changelog entry * Clean up partial files on any error
This fixes https://github.com/sourcegraph/sourcegraph/issues/15865
This fixes a bug where src-cli would leave partially-downloaded ZIP
files behind when the user hit Ctrl-C while a download was ongoing.
The
fetchRepositoryArchivewould then run into a context-cancelationerror when doing the request or the
io.Copyand not clean up the file.The code here checks whether the cause of the error is a cancelation
and, if so, cleans up the zip file.