Merge pull request #3164 from MichaelEischer/improve-context-cancel

Improve context cancel handling in archiver and backends

changelog/unreleased/issue-3151

@@ -0,0 +1,9 @@
+Bugfix: Never create invalid snapshots on backup interruption
+
+When canceling a backup run in the wrong moment it was possible that
+restic created a snapshot with an invalid "null" tree. This caused
+check and other operations to fail. The backup command now properly
+handles interruptions and never saves a snapshot in that case.
+
+https://github.com/restic/restic/issues/3151
+https://github.com/restic/restic/pull/3164

internal/archiver/archiver.go

@@ -178,6 +178,10 @@ func (arch *Archiver) saveTree(ctx context.Context, t *restic.Tree) (restic.ID,
 		s.TreeBlobs++
 		s.TreeSize += uint64(len(buf))
 	}
+	// The context was canceled in the meantime, res.ID() might be invalid
+	if ctx.Err() != nil {
+		return restic.ID{}, s, ctx.Err()
+	}
 	return res.ID(), s, nil
 }
 
@@ -803,7 +807,8 @@ func (arch *Archiver) Snapshot(ctx context.Context, targets []string, opts Snaps
 	t.Kill(nil)
 	werr := t.Wait()
 	debug.Log("err is %v, werr is %v", err, werr)
-	if err == nil || errors.Cause(err) == context.Canceled {
+	// Use werr when it might contain a more specific error than "context canceled"
+	if err == nil || (errors.Cause(err) == context.Canceled && werr != nil) {
 		err = werr
 	}
 

internal/archiver/archiver_test.go

@@ -3,6 +3,7 @@ package archiver
 import (
 	"bytes"
 	"context"
+	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -15,6 +16,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/restic/restic/internal/backend/mem"
 	"github.com/restic/restic/internal/checker"
 	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/fs"
@@ -1814,6 +1816,69 @@ func TestArchiverErrorReporting(t *testing.T) {
 	}
 }
 
+type noCancelBackend struct {
+	restic.Backend
+}
+
+func (c *noCancelBackend) Test(ctx context.Context, h restic.Handle) (bool, error) {
+	return c.Backend.Test(context.Background(), h)
+}
+
+func (c *noCancelBackend) Remove(ctx context.Context, h restic.Handle) error {
+	return c.Backend.Remove(context.Background(), h)
+}
+
+func (c *noCancelBackend) Save(ctx context.Context, h restic.Handle, rd restic.RewindReader) error {
+	return c.Backend.Save(context.Background(), h, rd)
+}
+
+func (c *noCancelBackend) Load(ctx context.Context, h restic.Handle, length int, offset int64, fn func(rd io.Reader) error) error {
+	return c.Backend.Load(context.Background(), h, length, offset, fn)
+}
+
+func (c *noCancelBackend) Stat(ctx context.Context, h restic.Handle) (restic.FileInfo, error) {
+	return c.Backend.Stat(context.Background(), h)
+}
+
+func (c *noCancelBackend) List(ctx context.Context, t restic.FileType, fn func(restic.FileInfo) error) error {
+	return c.Backend.List(context.Background(), t, fn)
+}
+
+func (c *noCancelBackend) Delete(ctx context.Context) error {
+	return c.Backend.Delete(context.Background())
+}
+
+func TestArchiverContextCanceled(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	tempdir, removeTempdir := restictest.TempDir(t)
+	TestCreateFiles(t, tempdir, TestDir{
+		"targetfile": TestFile{Content: "foobar"},
+	})
+	defer removeTempdir()
+
+	// Ensure that the archiver itself reports the canceled context and not just the backend
+	repo, _ := repository.TestRepositoryWithBackend(t, &noCancelBackend{mem.New()})
+
+	back := restictest.Chdir(t, tempdir)
+	defer back()
+
+	arch := New(repo, fs.Track{FS: fs.Local{}}, Options{})
+
+	_, snapshotID, err := arch.Snapshot(ctx, []string{"."}, SnapshotOptions{Time: time.Now()})
+
+	if err != nil {
+		t.Logf("found expected error (%v)", err)
+		return
+	}
+	if snapshotID.IsNull() {
+		t.Fatalf("no error returned but found null id")
+	}
+
+	t.Fatalf("expected error not returned by archiver")
+}
+
 // TrackFS keeps track which files are opened. For some files, an error is injected.
 type TrackFS struct {
 	fs.FS

internal/backend/backend_retry.go

@@ -33,6 +33,16 @@ func NewRetryBackend(be restic.Backend, maxTries int, report func(string, error,
 }
 
 func (be *RetryBackend) retry(ctx context.Context, msg string, f func() error) error {
+	// Don't do anything when called with an already cancelled context. There would be
+	// no retries in that case either, so be consistent and abort always.
+	// This enforces a strict contract for backend methods: Using a cancelled context
+	// will prevent any backup repository modifications. This simplifies ensuring that
+	// a backup repository is not modified any further after a context was cancelled.
+	// The 'local' backend for example does not provide this guarantee on its own.
+	if ctx.Err() != nil {
+		return ctx.Err()
+	}
+
 	err := backoff.RetryNotify(f,
 		backoff.WithContext(backoff.WithMaxRetries(backoff.NewExponentialBackOff(), uint64(be.MaxTries)), ctx),
 		func(err error, d time.Duration) {

internal/backend/backend_retry_test.go

@@ -236,3 +236,38 @@ func TestBackendLoadRetry(t *testing.T) {
 	test.Equals(t, data, buf)
 	test.Equals(t, 2, attempt)
 }
+
+func assertIsCanceled(t *testing.T, err error) {
+	test.Assert(t, err == context.Canceled, "got unexpected err %v", err)
+}
+
+func TestBackendCanceledContext(t *testing.T) {
+	// unimplemented mock backend functions return an error by default
+	// check that we received the expected context canceled error instead
+	retryBackend := NewRetryBackend(mock.NewBackend(), 2, nil)
+	h := restic.Handle{Type: restic.PackFile, Name: restic.NewRandomID().String()}
+
+	// create an already canceled context
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	_, err := retryBackend.Test(ctx, h)
+	assertIsCanceled(t, err)
+	_, err = retryBackend.Stat(ctx, h)
+	assertIsCanceled(t, err)
+
+	err = retryBackend.Save(ctx, h, restic.NewByteReader([]byte{}))
+	assertIsCanceled(t, err)
+	err = retryBackend.Remove(ctx, h)
+	assertIsCanceled(t, err)
+	err = retryBackend.Load(ctx, restic.Handle{}, 0, 0, func(rd io.Reader) (err error) {
+		return nil
+	})
+	assertIsCanceled(t, err)
+	err = retryBackend.List(ctx, restic.PackFile, func(restic.FileInfo) error {
+		return nil
+	})
+	assertIsCanceled(t, err)
+
+	// don't test "Delete" as it is not used by normal code
+}