Stop replacing files in LFS storage when downloading them concurrently on Windows

On Windows, there is no way to replace file atomically. Instead, MoveFileExA:

1. Calls CreateFileA(access=Delete, shareMode=Delete)
2. Calls SetRenameInformationFile to rename file
3. Calls CloseFile

The problem is that if parallel process attempts to open destination file for reading between
steps 2 and 3, it will try to do that without shareMode=Delete and will hit a SHARING_VIOLATION error.

In practice, this race condition results in:

  Smudge error: Error opening media file.: open .git\lfs\objects\<sha>: The process cannot access the file because it is being used by another process.

There are two solutions here:

 1. Do not overwrite file if it already exists
 2. Retry reading from file if got a SHARING_VIOLATION

This commit implements option 1.

Fixes #2825 (for Windows, other OSes are race-free already). Also see #3813 and #3826.

tools/util_darwin.go

@@ -141,3 +141,7 @@ func cloneFileSyscall(dst, src string) *CloneFileError {
 
 	return nil
 }
+
+func TryRename(oldname, newname string) error {
+	return RenameFileCopyPermissions(oldname, newname)
+}

tools/util_generic.go

@@ -21,3 +21,7 @@ func CloneFile(writer io.Writer, reader io.Reader) (bool, error) {
 func CloneFileByPath(_, _ string) (bool, error) {
 	return false, nil
 }
+
+func TryRename(oldname, newname string) error {
+	return RenameFileCopyPermissions(oldname, newname)
+}

tools/util_linux.go

@@ -71,3 +71,7 @@ func CloneFileByPath(dst, src string) (bool, error) {
 
 	return CloneFile(dstFile, srcFile)
 }
+
+func TryRename(oldname, newname string) error {
+	return RenameFileCopyPermissions(oldname, newname)
+}

tools/util_windows.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"unsafe"
 
 	"golang.org/x/sys/windows"
@@ -156,3 +157,26 @@ func roundUp(value, base int64) int64 {
 
 	return value - mod + base
 }
+
+// This is a simplified variant of fixLongPath from file_windows.go. Unfortunately, that function is not public
+func toSafePath(path string) (*uint16, error) {
+	abspath, err := filepath.Abs(path)
+	if err != nil {
+		return nil, err
+	}
+
+	return windows.UTF16PtrFromString(`\\?\` + abspath)
+}
+
+// This is almost the same as os.Rename but doesn't overwrite destination if it already exists
+func TryRename(oldname, newname string) error {
+	from, err := toSafePath(oldname)
+	if err != nil {
+		return err
+	}
+	to, err := toSafePath(newname)
+	if err != nil {
+		return err
+	}
+	return windows.MoveFileEx(from, to, 0)
+}

tools/util_windows_test.go

@@ -109,3 +109,61 @@ func fillFile(target *os.File, size int64) (hash string, err error) {
 
 	return hex.EncodeToString(sha.Sum(nil)), nil
 }
+
+func TestTryRenameDestExists(t *testing.T) {
+	source, err := ioutil.TempFile("", "source")
+	assert.NoError(t, err)
+	assert.NoError(t, source.Close())
+	defer os.Remove(source.Name())
+
+	sourceData := []byte("source")
+	assert.NoError(t, ioutil.WriteFile(source.Name(), sourceData, 0644))
+
+	dest, err := ioutil.TempFile("", "dest")
+	assert.NoError(t, err)
+	defer os.Remove(dest.Name())
+	assert.NoError(t, dest.Close())
+
+	destData := []byte("dest")
+	assert.NoError(t, ioutil.WriteFile(dest.Name(), destData, 0644))
+
+	// Perform rename
+	err = TryRename(source.Name(), dest.Name())
+	assert.True(t, os.IsExist(err))
+
+	sourceData2, err := ioutil.ReadFile(source.Name())
+	assert.NoError(t, err)
+	assert.Equal(t, sourceData, sourceData2)
+
+	destData2, err := ioutil.ReadFile(dest.Name())
+	assert.NoError(t, err)
+	assert.Equal(t, destData, destData2)
+}
+
+func TestTryRename(t *testing.T) {
+	source, err := ioutil.TempFile("", "source")
+	assert.NoError(t, err)
+	assert.NoError(t, source.Close())
+	defer os.Remove(source.Name())
+
+	sourceData := []byte("source")
+	assert.NoError(t, ioutil.WriteFile(source.Name(), sourceData, 0644))
+
+	dest, err := ioutil.TempFile("", "dest")
+	assert.NoError(t, err)
+	defer os.Remove(dest.Name())
+	assert.NoError(t, dest.Close())
+
+	// Remove destination file
+	assert.NoError(t, os.Remove(dest.Name()))
+
+	// Perform rename
+	assert.NoError(t, TryRename(source.Name(), dest.Name()))
+
+	_, err = os.Stat(source.Name())
+	assert.True(t, os.IsNotExist(err))
+
+	destData, err := ioutil.ReadFile(dest.Name())
+	assert.NoError(t, err)
+	assert.Equal(t, sourceData, destData)
+}

tq/basic_download.go

@@ -100,7 +100,7 @@ func (a *basicDownloadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progr
 		f.Close()
 		// Rename file so next download can resume from where we stopped.
 		// No error checking here, if rename fails then file will be deleted and there just will be no download resuming
-		os.Rename(f.Name(), a.downloadFilename(t))
+		tools.TryRename(f.Name(), a.downloadFilename(t))
 	}
 
 	return err
@@ -254,12 +254,15 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk
 		return fmt.Errorf("can't close tempfile %q: %v", dlfilename, err)
 	}
 
-	err = tools.RenameFileCopyPermissions(dlfilename, t.Path)
-	if _, err2 := os.Stat(t.Path); err2 == nil {
-		// Target file already exists, possibly was downloaded by other git-lfs process
-		return nil
+	err = tools.TryRename(dlfilename, t.Path)
+	// If rename failed because file already exists, we do not treat it as error
+	// This can happen when multiple git-lfs processes are fetching files concurrently on Windows
+	// Note that dlfilename needs to be handled regardless of TryRename success
+	if err != nil && !os.IsExist(err) {
+		return err
 	}
-	return err
+
+	return nil
 }
 
 func configureBasicDownloadAdapter(m *Manifest) {