Merge pull request #3436 from greatroar/local-save-tmp

Save files under temporary name in local backend

changelog/unreleased/pull-3436

@@ -0,0 +1,12 @@
+Enhancement: Improve local backend's resilience to (system) crashes
+
+Restic now ensures that files stored using the `local` backend are created
+atomically (that is, files are either stored completely or not at all). This
+ensures that no incomplete files are left behind even if restic is terminated
+while writing a file.
+
+In addition, restic now tries to ensure that the directory in the repository
+which contains a newly uploaded file is also written to disk. This can prevent
+missing files if the system crashes or the disk is not properly unmounted.
+
+https://github.com/restic/restic/pull/3436

internal/backend/local/local.go

@@ -3,6 +3,7 @@ package local
 import (
 	"context"
 	"io"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"syscall"
@@ -88,7 +89,8 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade
 		return backoff.Permanent(err)
 	}
 
-	filename := b.Filename(h)
+	finalname := b.Filename(h)
+	dir := filepath.Dir(finalname)
 
 	defer func() {
 		// Mark non-retriable errors as such
@@ -97,19 +99,20 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade
 		}
 	}()
 
-	// create new file
-	f, err := openFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File)
+	// Create new file with a temporary name.
+	tmpname := filepath.Base(finalname) + "-tmp-"
+	f, err := tempFile(dir, tmpname)
 
 	if b.IsNotExist(err) {
 		debug.Log("error %v: creating dir", err)
 
 		// error is caused by a missing directory, try to create it
-		mkdirErr := os.MkdirAll(filepath.Dir(filename), backend.Modes.Dir)
+		mkdirErr := fs.MkdirAll(dir, backend.Modes.Dir)
 		if mkdirErr != nil {
-			debug.Log("error creating dir %v: %v", filepath.Dir(filename), mkdirErr)
+			debug.Log("error creating dir %v: %v", dir, mkdirErr)
 		} else {
 			// try again
-			f, err = openFile(filename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, backend.Modes.File)
+			f, err = tempFile(dir, tmpname)
 		}
 	}
 
@@ -117,37 +120,54 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade
 		return errors.WithStack(err)
 	}
 
+	defer func(f *os.File) {
+		if err != nil {
+			_ = f.Close() // Double Close is harmless.
+			// Remove after Rename is harmless: we embed the final name in the
+			// temporary's name and no other goroutine will get the same data to
+			// Save, so the temporary name should never be reused by another
+			// goroutine.
+			_ = fs.Remove(f.Name())
+		}
+	}(f)
+
 	// save data, then sync
 	wbytes, err := io.Copy(f, rd)
 	if err != nil {
-		_ = f.Close()
 		return errors.WithStack(err)
 	}
 	// sanity check
 	if wbytes != rd.Length() {
-		_ = f.Close()
 		return errors.Errorf("wrote %d bytes instead of the expected %d bytes", wbytes, rd.Length())
 	}
 
-	if err = f.Sync(); err != nil {
-		pathErr, ok := err.(*os.PathError)
-		isNotSupported := ok && pathErr.Op == "sync" && pathErr.Err == syscall.ENOTSUP
-		// ignore error if filesystem does not support the sync operation
-		if !isNotSupported {
-			_ = f.Close()
+	// Ignore error if filesystem does not support fsync.
+	err = f.Sync()
+	syncNotSup := errors.Is(err, syscall.ENOTSUP)
+	if err != nil && !syncNotSup {
 		return errors.WithStack(err)
 	}
-	}
 
-	err = f.Close()
+	// Close, then rename. Windows doesn't like the reverse order.
+	if err = f.Close(); err != nil {
+		return errors.WithStack(err)
+	}
+	if err = os.Rename(f.Name(), finalname); err != nil {
+		return errors.WithStack(err)
+	}
+
+	// Now sync the directory to commit the Rename.
+	if !syncNotSup {
+		err = fsyncDir(dir)
 		if err != nil {
 			return errors.WithStack(err)
 		}
+	}
 
 	// try to mark file as read-only to avoid accidential modifications
 	// ignore if the operation fails as some filesystems don't allow the chmod call
 	// e.g. exfat and network file systems with certain mount options
-	err = setFileReadonly(filename, backend.Modes.File)
+	err = setFileReadonly(finalname, backend.Modes.File)
 	if err != nil && !os.IsPermission(err) {
 		return errors.WithStack(err)
 	}
@@ -155,7 +175,7 @@ func (b *Local) Save(ctx context.Context, h restic.Handle, rd restic.RewindReade
 	return nil
 }
 
-var openFile = fs.OpenFile // Overridden by test.
+var tempFile = ioutil.TempFile // Overridden by test.
 
 // Load runs fn with a reader that yields the contents of the file at h at the
 // given offset.

internal/backend/local/local_internal_test.go

@@ -3,6 +3,7 @@ package local
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"syscall"
 	"testing"
@@ -14,15 +15,13 @@ import (
 )
 
 func TestNoSpacePermanent(t *testing.T) {
-	oldOpenFile := openFile
+	oldTempFile := tempFile
 	defer func() {
-		openFile = oldOpenFile
+		tempFile = oldTempFile
 	}()
 
-	openFile = func(name string, flags int, mode os.FileMode) (*os.File, error) {
-		// The actual error from os.OpenFile is *os.PathError.
-		// Other functions called inside Save may return *os.SyscallError.
-		return nil, os.NewSyscallError("open", syscall.ENOSPC)
+	tempFile = func(_, _ string) (*os.File, error) {
+		return nil, fmt.Errorf("not creating tempfile, %w", syscall.ENOSPC)
 	}
 
 	dir, cleanup := rtest.TempDir(t)

internal/backend/local/local_unix.go

@@ -3,11 +3,33 @@
 package local
 
 import (
+	"errors"
 	"os"
+	"syscall"
 
 	"github.com/restic/restic/internal/fs"
 )
 
+// fsyncDir flushes changes to the directory dir.
+func fsyncDir(dir string) error {
+	d, err := os.Open(dir)
+	if err != nil {
+		return err
+	}
+
+	err = d.Sync()
+	if errors.Is(err, syscall.ENOTSUP) {
+		err = nil
+	}
+
+	cerr := d.Close()
+	if err == nil {
+		err = cerr
+	}
+
+	return err
+}
+
 // set file to readonly
 func setFileReadonly(f string, mode os.FileMode) error {
 	return fs.Chmod(f, mode&^0222)

internal/backend/local/local_windows.go

@@ -4,6 +4,9 @@ import (
 	"os"
 )
 
+// Can't explicitly flush directory changes on Windows.
+func fsyncDir(dir string) error { return nil }
+
 // We don't modify read-only on windows,
 // since it will make us unable to delete the file,
 // and this isn't common practice on this platform.