From 736e964317a9e323a29201f07940dfe8380d193d Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 13 Dec 2020 20:01:16 +0100 Subject: [PATCH 1/6] archiver: Don't loose error if background context is canceled A canceled background context lets the blob/tree/fileSavers exit without reporting an error. The error handling previously replaced a 'context canceled' error received by the main backup method with the error reported by the savers. However, in case of a canceled background context that error is nil, causing restic to loose the error and save a snapshot with a nil tree. --- internal/archiver/archiver.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 5573f1722..2e4af5cd8 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -803,7 +803,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 } From fc60b560ba3694d47f21a8bf690eb415e8122847 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 13 Dec 2020 20:06:44 +0100 Subject: [PATCH 2/6] archiver: Let saveTree report a canceled context as an error If the context was canceled then saveTree might receive a treeID or not depending on the timing. This could cause saveTree to incorrectly return a nil treeID as valid. Fix this always returning an error when the context was canceled in the meantime. --- internal/archiver/archiver.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index 2e4af5cd8..4992d439b 100644 --- a/internal/archiver/archiver.go +++ b/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 } From e483b63c40e72325f495c030f517233aed383c0b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 13 Dec 2020 20:11:01 +0100 Subject: [PATCH 3/6] retrybackend: Fail operations when context is already canceled Depending on the used backend, operations started with a canceled context may fail or not. For example the local backend still works in large parts when called with a canceled context. Backends transfering data via http don't work. It is also not possible to retry failed operations in that state as the RetryBackend will abort with a 'context canceled' error. Ensure uniform behavior of all backends by checking for a canceled context by checking for a canceled context as a first step in the RetryBackend. This ensures uniform behavior across all backends, as backends are always wrapped in a RetryBackend. --- internal/backend/backend_retry.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/backend/backend_retry.go b/internal/backend/backend_retry.go index d47be2396..5d7a58b64 100644 --- a/internal/backend/backend_retry.go +++ b/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) { From 08b7f2b58d41dd517cad2282ee5ede567bc7292d Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 28 Dec 2020 20:45:53 +0100 Subject: [PATCH 4/6] archiver: test that context canceled error is not dropped --- internal/archiver/archiver_test.go | 65 ++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 38641cff3..afd8e0f8a 100644 --- a/internal/archiver/archiver_test.go +++ b/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 From d0ca8fb0b85df38b7004c517b8d83de724fa7a56 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 28 Dec 2020 20:47:11 +0100 Subject: [PATCH 5/6] backend: test that a canceled context prevents RetryBackend operations --- internal/backend/backend_retry_test.go | 35 ++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/internal/backend/backend_retry_test.go b/internal/backend/backend_retry_test.go index 1cc18df1f..9c9a6b708 100644 --- a/internal/backend/backend_retry_test.go +++ b/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 +} From ca073178159dc3064f9e52f30206b22ae6946bac Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Mon, 28 Dec 2020 21:06:22 +0100 Subject: [PATCH 6/6] add changelog --- changelog/unreleased/issue-3151 | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 changelog/unreleased/issue-3151 diff --git a/changelog/unreleased/issue-3151 b/changelog/unreleased/issue-3151 new file mode 100644 index 000000000..d10aaba2c --- /dev/null +++ b/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