From aeb7eb245c679380b2867601d50c227f2d86fc92 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@fau.de>
Date: Sat, 11 May 2024 20:25:04 +0200
Subject: [PATCH] retry: do not retry permanent errors

This is currently gated behind a feature flag as some unexpected
interactions might show up in the wild.
---
 internal/backend/retry/backend_retry.go      | 19 +++++++----
 internal/backend/retry/backend_retry_test.go | 34 ++++++++++++++++++--
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/internal/backend/retry/backend_retry.go b/internal/backend/retry/backend_retry.go
index c63338fb6..4f25e0c7c 100644
--- a/internal/backend/retry/backend_retry.go
+++ b/internal/backend/retry/backend_retry.go
@@ -2,6 +2,7 @@ package retry
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"time"
@@ -9,6 +10,7 @@ import (
 	"github.com/cenkalti/backoff/v4"
 	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/debug"
+	"github.com/restic/restic/internal/feature"
 )
 
 // Backend retries operations on the backend in case of an error with a
@@ -74,7 +76,16 @@ func (be *Backend) retry(ctx context.Context, msg string, f func() error) error
 		bo.InitialInterval = 1 * time.Millisecond
 	}
 
-	err := retryNotifyErrorWithSuccess(f,
+	err := retryNotifyErrorWithSuccess(
+		func() error {
+			err := f()
+			// don't retry permanent errors as those very likely cannot be fixed by retrying
+			// TODO remove IsNotExist(err) special cases when removing the feature flag
+			if feature.Flag.Enabled(feature.BackendErrorRedesign) && !errors.Is(err, &backoff.PermanentError{}) && be.Backend.IsPermanentError(err) {
+				return backoff.Permanent(err)
+			}
+			return err
+		},
 		backoff.WithContext(backoff.WithMaxRetries(bo, uint64(be.MaxTries)), ctx),
 		func(err error, d time.Duration) {
 			if be.Report != nil {
@@ -128,11 +139,7 @@ func (be *Backend) Save(ctx context.Context, h backend.Handle, rd backend.Rewind
 func (be *Backend) Load(ctx context.Context, h backend.Handle, length int, offset int64, consumer func(rd io.Reader) error) (err error) {
 	return be.retry(ctx, fmt.Sprintf("Load(%v, %v, %v)", h, length, offset),
 		func() error {
-			err := be.Backend.Load(ctx, h, length, offset, consumer)
-			if be.Backend.IsNotExist(err) {
-				return backoff.Permanent(err)
-			}
-			return err
+			return be.Backend.Load(ctx, h, length, offset, consumer)
 		})
 }
 
diff --git a/internal/backend/retry/backend_retry_test.go b/internal/backend/retry/backend_retry_test.go
index 405cdfa59..80964fb37 100644
--- a/internal/backend/retry/backend_retry_test.go
+++ b/internal/backend/retry/backend_retry_test.go
@@ -289,7 +289,7 @@ func TestBackendLoadNotExists(t *testing.T) {
 		}
 		return nil, notFound
 	}
-	be.IsNotExistFn = func(err error) bool {
+	be.IsPermanentErrorFn = func(err error) bool {
 		return errors.Is(err, notFound)
 	}
 
@@ -299,7 +299,7 @@ func TestBackendLoadNotExists(t *testing.T) {
 	err := retryBackend.Load(context.TODO(), backend.Handle{}, 0, 0, func(rd io.Reader) (err error) {
 		return nil
 	})
-	test.Assert(t, be.IsNotExistFn(err), "unexpected error %v", err)
+	test.Assert(t, be.IsPermanentErrorFn(err), "unexpected error %v", err)
 	test.Equals(t, 1, attempt)
 }
 
@@ -329,6 +329,36 @@ func TestBackendStatNotExists(t *testing.T) {
 	test.Equals(t, 1, attempt)
 }
 
+func TestBackendRetryPermanent(t *testing.T) {
+	// retry should not retry if the error matches IsPermanentError
+	notFound := errors.New("not found")
+	attempt := 0
+
+	be := mock.NewBackend()
+	be.IsPermanentErrorFn = func(err error) bool {
+		return errors.Is(err, notFound)
+	}
+
+	TestFastRetries(t)
+	retryBackend := New(be, 2, nil, nil)
+	err := retryBackend.retry(context.TODO(), "test", func() error {
+		attempt++
+		return notFound
+	})
+
+	test.Assert(t, be.IsPermanentErrorFn(err), "unexpected error %v", err)
+	test.Equals(t, 1, attempt)
+
+	attempt = 0
+	err = retryBackend.retry(context.TODO(), "test", func() error {
+		attempt++
+		return errors.New("something")
+	})
+	test.Assert(t, !be.IsPermanentErrorFn(err), "error unexpectedly considered permanent %v", err)
+	test.Equals(t, 3, attempt)
+
+}
+
 func assertIsCanceled(t *testing.T, err error) {
 	test.Assert(t, err == context.Canceled, "got unexpected err %v", err)
 }