Merge pull request #3885 from MichaelEischer/delete-fixes

Improve reliability of upload retries and B2 file deletions

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

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 {

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

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"