diff --git a/changelog/unreleased/issue-3161 b/changelog/unreleased/issue-3161 new file mode 100644 index 000000000..ec18b6639 --- /dev/null +++ b/changelog/unreleased/issue-3161 @@ -0,0 +1,13 @@ +Bugfix: Reliably delete files on Backblaze B2 + +Restic used to only delete the latest version of files stored in B2. In most +cases this works well as there is only a single version. However, due to +retries while uploading it is possible for multiple file versions to be stored +at B2. This can lead to various problems if files still exist that should have +been deleted. + +We have changed the implementation to delete all of them. This doubles the +number of Class B transactions necessary to delete files. + +https://github.com/restic/restic/issues/3161 +https://github.com/restic/restic/pull/3885 diff --git a/internal/backend/b2/b2.go b/internal/backend/b2/b2.go index 150d396d5..19328706d 100644 --- a/internal/backend/b2/b2.go +++ b/internal/backend/b2/b2.go @@ -27,7 +27,8 @@ type b2Backend struct { sem sema.Semaphore } -const defaultListMaxItems = 1000 +// Billing happens in 1000 item granlarity, but we are more interested in reducing the number of network round trips +const defaultListMaxItems = 10 * 1000 // ensure statically that *b2Backend implements restic.Backend. var _ restic.Backend = &b2Backend{} @@ -274,14 +275,22 @@ func (be *b2Backend) Remove(ctx context.Context, h restic.Handle) error { be.sem.GetToken() defer be.sem.ReleaseToken() - obj := be.bucket.Object(be.Filename(h)) - err := obj.Delete(ctx) - // consider a file as removed if b2 informs us that it does not exist - if b2.IsNotExist(err) { - return nil + // the retry backend will also repeat the remove method up to 10 times + for i := 0; i < 3; i++ { + obj := be.bucket.Object(be.Filename(h)) + err := obj.Delete(ctx) + if err == nil { + // keep deleting until we are sure that no leftover file versions exist + continue + } + // consider a file as removed if b2 informs us that it does not exist + if b2.IsNotExist(err) { + return nil + } + return errors.Wrap(err, "Delete") } - return errors.Wrap(err, "Delete") + return errors.New("failed to delete all file versions") } type semLocker struct { diff --git a/internal/backend/backend_retry.go b/internal/backend/backend_retry.go index 5d7a58b64..fe0d34799 100644 --- a/internal/backend/backend_retry.go +++ b/internal/backend/backend_retry.go @@ -68,10 +68,16 @@ func (be *RetryBackend) Save(ctx context.Context, h restic.Handle, rd restic.Rew return nil } - debug.Log("Save(%v) failed with error, removing file: %v", h, err) - rerr := be.Backend.Remove(ctx, h) - if rerr != nil { - debug.Log("Remove(%v) returned error: %v", h, err) + if be.Backend.HasAtomicReplace() { + debug.Log("Save(%v) failed with error: %v", h, err) + // there is no need to remove files from backends which can atomically replace files + // in fact if something goes wrong at the backend side the delete operation might delete the wrong instance of the file + } else { + debug.Log("Save(%v) failed with error, removing file: %v", h, err) + rerr := be.Backend.Remove(ctx, h) + if rerr != nil { + debug.Log("Remove(%v) returned error: %v", h, err) + } } // return original error diff --git a/internal/backend/backend_retry_test.go b/internal/backend/backend_retry_test.go index e8f4d7315..cd58b9d77 100644 --- a/internal/backend/backend_retry_test.go +++ b/internal/backend/backend_retry_test.go @@ -50,6 +50,36 @@ func TestBackendSaveRetry(t *testing.T) { } } +func TestBackendSaveRetryAtomic(t *testing.T) { + errcount := 0 + calledRemove := false + be := &mock.Backend{ + SaveFn: func(ctx context.Context, h restic.Handle, rd restic.RewindReader) error { + if errcount == 0 { + errcount++ + return errors.New("injected error") + } + return nil + }, + RemoveFn: func(ctx context.Context, h restic.Handle) error { + calledRemove = true + return nil + }, + HasAtomicReplaceFn: func() bool { return true }, + } + + retryBackend := NewRetryBackend(be, 10, nil) + + data := test.Random(23, 5*1024*1024+11241) + err := retryBackend.Save(context.TODO(), restic.Handle{}, restic.NewByteReader(data, be.Hasher())) + if err != nil { + t.Fatal(err) + } + if calledRemove { + t.Fatal("remove must not be called") + } +} + func TestBackendListRetry(t *testing.T) { const ( ID1 = "id1"