[Windows] Rotate (rename) files with retries (#333)

* Add unit test

* Implement a retrying rename

* Running goimports

* Assert on no error

* Add variadic options to SafeFileRotate

* Inlining implementation

* Add explanatory comment

* Fixing up license header

* Reducing visibility

* Fix logic for non-retrying rename

* Enable retries by default on Windows

* Revert "Enable retries by default on Windows"

This reverts commit 1a887d7.

* Retry by default for Windows

Author: ycombinator

file/helper_aix.go

@@ -23,10 +23,14 @@ import (
 )
 
 // SafeFileRotate safely rotates an existing file under path and replaces it with the tempfile
-func SafeFileRotate(path, tempfile string) error {
+func SafeFileRotate(path, tempfile string, opts ...RotateOpt) error {
+	options := rotateOpts{}
+	for _, opt := range opts {
+		opt(&options)
+	}
 
-	if e := os.Rename(tempfile, path); e != nil {
-		return e
+	if err := rename(tempfile, path, options); err != nil {
+		return err
 	}
 
 	// best-effort fsync on parent directory. The fsync is required by some

file/helper_common.go

@@ -0,0 +1,62 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package file
+
+import (
+	"fmt"
+	"os"
+	"time"
+)
+
+type rotateOpts struct {
+	renameRetryDuration time.Duration
+	renameRetryInterval time.Duration
+}
+
+type RotateOpt func(*rotateOpts)
+
+func WithRenameRetries(duration, interval time.Duration) RotateOpt {
+	return func(opts *rotateOpts) {
+		opts.renameRetryDuration = duration
+		opts.renameRetryInterval = interval
+	}
+}
+
+func rename(src, dst string, options rotateOpts) error {
+	// Perform a regular (non-retrying) rename unless all retry options are specified.
+	if options.renameRetryDuration == 0 || options.renameRetryInterval == 0 {
+		return os.Rename(src, dst)
+	}
+
+	// Attempt rename with retries every options.RenameRetryInterval until options.RenameRetryDuration
+	// has elapsed. This is useful in cases where the destination file may be locked or in use.
+	var err error
+	for start := time.Now(); time.Since(start) < options.renameRetryDuration; time.Sleep(options.renameRetryInterval) {
+		err = os.Rename(src, dst)
+		if err == nil {
+			// Rename succeeded; no more retries needed
+			return nil
+		}
+	}
+
+	if err != nil {
+		return fmt.Errorf("failed to rename %s to %s after %v: %w", src, dst, options.renameRetryDuration, err)
+	}
+
+	return nil
+}

file/helper_other.go

@@ -25,10 +25,14 @@ import (
 )
 
 // SafeFileRotate safely rotates an existing file under path and replaces it with the tempfile
-func SafeFileRotate(path, tempfile string) error {
+func SafeFileRotate(path, tempfile string, opts ...RotateOpt) error {
+	options := rotateOpts{}
+	for _, opt := range opts {
+		opt(&options)
+	}
 
-	if e := os.Rename(tempfile, path); e != nil {
-		return e
+	if err := rename(tempfile, path, options); err != nil {
+		return err
 	}
 
 	// best-effort fsync on parent directory. The fsync is required by some

file/helper_windows.go

@@ -20,12 +20,25 @@ package file
 import (
 	"os"
 	"path/filepath"
+	"time"
 )
 
+const windowsRenameRetryInterval = 50 * time.Millisecond
+const windowsRenameRetryDuration = 2 * time.Second
+
 // SafeFileRotate safely rotates an existing file under path and replaces it with the tempfile
-func SafeFileRotate(path, tempfile string) error {
+func SafeFileRotate(path, tempfile string, opts ...RotateOpt) error {
+	// On Windows, retry the rename operation by default. This is useful in cases where
+	// path, the destination file, may be locked or in use.
+	options := rotateOpts{
+		renameRetryDuration: windowsRenameRetryDuration,
+		renameRetryInterval: windowsRenameRetryInterval,
+	}
+	for _, opt := range opts {
+		opt(&options)
+	}
+
 	old := path + ".old"
-	var e error
 
 	// In Windows, one cannot rename a file if the destination already exists, at least
 	// not with using the os.Rename function that Golang offers.
@@ -37,8 +50,8 @@ func SafeFileRotate(path, tempfile string) error {
 	// ignore error in case path doesn't exist
 	_ = os.Rename(path, old)
 
-	if e = os.Rename(tempfile, path); e != nil {
-		return e
+	if err := rename(tempfile, path, options); err != nil {
+		return err
 	}
 
 	// .old file will still exist if path file is already there, it should be removed

file/helper_windows_test.go

@@ -0,0 +1,62 @@
+// Licensed to Elasticsearch B.V. under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Elasticsearch B.V. licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package file
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestSafeFileRotate creates two files, dest and src, and calls
+// SafeFileRotate to replace dest with src. However, before the test
+// makes that call, it deliberately keeps a handle open on dest for
+// a short period of time to ensure that the rotation takes place anyway
+// after the handle has been released.
+func TestSafeFileRotate(t *testing.T) {
+	// Create destination file
+	tmpDir := t.TempDir()
+	dest := filepath.Join(tmpDir, "dest.txt")
+	err := os.WriteFile(dest, []byte("existing content"), 0644)
+	require.NoError(t, err)
+
+	// Create source file
+	src := filepath.Join(tmpDir, "src.txt")
+	err = os.WriteFile(src, []byte("new content"), 0644)
+	require.NoError(t, err)
+
+	// Open handle on dest file for 1.5 seconds
+	destFile, err := os.Open(dest)
+	require.NoError(t, err)
+	time.AfterFunc(1500*time.Millisecond, func() {
+		destFile.Close() // Close the handle after 1.5 seconds
+	})
+	defer destFile.Close()
+
+	// Try to replace dest with new
+	err = SafeFileRotate(dest, src, WithRenameRetries(2*time.Second, 100*time.Millisecond))
+	require.NoError(t, err)
+
+	// Check that dest file has been replaced with new file
+	data, err := os.ReadFile(dest)
+	require.NoError(t, err)
+	require.Equal(t, "new content", string(data))
+}