batches: Make sure no corrupted repozips are used #817
Conversation
When the download of a zip was aborted, it could've left behind a corrupted archive zip. This fixes it by first storing it to a temporary location and then creating the actual file atomically by doing os.Rename.
internal/batches/repozip/fetcher.go
Outdated
| tmpFileDir := fmt.Sprintf("%s.%d.tmp", dest, rand.Int()) | ||
| // Ensure the file doesn't exist. | ||
| _ = os.Remove(tmpFileDir) | ||
| f, err := os.Create(tmpFileDir) |
There was a problem hiding this comment.
We should use os.CreateTemp here — it does essentially the same thing, but in a slightly safer way. (We also need to ensure it creates the file in the same directory that it will end up in, as I think you already know, since rename operations usually can't cross filesystem boundaries.)
There was a problem hiding this comment.
I think we'll need a defer func(path string) { _ = os.Remove(path) }(f.Name()) after the CreateTemp call, since there's a possible return path before the Rename, but otherwise LGTM.
There was a problem hiding this comment.
Makes sense. I also made sure we close the file before we rename it, although on macOS at least that doesn't seem to make a difference.
There was a problem hiding this comment.
Yeah, I think that would only matter on Windows, and I'm not even sure there. No harm in being safe, though!
When the download of a zip was aborted, it could've left behind a corrupted archive zip. This fixes it by first storing it to a temporary location and then creating the actual file atomically by doing os.Rename.
When the download of a zip was aborted, it could've left behind a corrupted archive zip. This fixes it by first storing it to a temporary location and then creating the actual file atomically by doing os.Rename.
Closes https://github.com/sourcegraph/sourcegraph/issues/39743
Test plan
Verified things still work as expected.