From e2984227bb7462126bfa7f8632c180f6f96a3016 Mon Sep 17 00:00:00 2001
From: Nick Craig-Wood <nick@craig-wood.com>
Date: Wed, 8 Mar 2023 18:40:37 +0000
Subject: [PATCH] fs: fix race conditions in --max-delete and --max-delete-size

---
 fs/accounting/stats.go             | 35 +++++++++++++++++++++++-------
 fs/accounting/stats_groups_test.go |  6 +++--
 fs/operations/operations.go        | 13 +++--------
 fs/rc/rcserver/rcserver_test.go    |  6 +++--
 4 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/fs/accounting/stats.go b/fs/accounting/stats.go
index 8e1310483..358cf4a6f 100644
--- a/fs/accounting/stats.go
+++ b/fs/accounting/stats.go
@@ -3,6 +3,7 @@ package accounting
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -591,20 +592,38 @@ func (s *StatsInfo) HadRetryError() bool {
 	return s.retryError
 }
 
-// Deletes updates the stats for deletes
-func (s *StatsInfo) Deletes(deletes int64) int64 {
+var (
+	errMaxDelete     = fserrors.FatalError(errors.New("--max-delete threshold reached"))
+	errMaxDeleteSize = fserrors.FatalError(errors.New("--max-delete-size threshold reached"))
+)
+
+// DeleteFile updates the stats for deleting a file
+//
+// It may return fatal errors if the threshold for --max-delete or
+// --max-delete-size have been reached.
+func (s *StatsInfo) DeleteFile(ctx context.Context, size int64) error {
+	ci := fs.GetConfig(ctx)
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.deletes += deletes
-	return s.deletes
+	if size < 0 {
+		size = 0
+	}
+	if ci.MaxDelete >= 0 && s.deletes+1 > ci.MaxDelete {
+		return errMaxDelete
+	}
+	if ci.MaxDeleteSize >= 0 && s.deletesSize+size > int64(ci.MaxDeleteSize) {
+		return errMaxDeleteSize
+	}
+	s.deletes++
+	s.deletesSize += size
+	return nil
 }
 
-// DeletesSize updates the stats for deletes size
-func (s *StatsInfo) DeletesSize(deletesSize int64) int64 {
+// GetDeletes returns the number of deletes
+func (s *StatsInfo) GetDeletes() int64 {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.deletesSize += deletesSize
-	return s.deletesSize
+	return s.deletes
 }
 
 // DeletedDirs updates the stats for deletedDirs
diff --git a/fs/accounting/stats_groups_test.go b/fs/accounting/stats_groups_test.go
index ed04d134e..dda9dccef 100644
--- a/fs/accounting/stats_groups_test.go
+++ b/fs/accounting/stats_groups_test.go
@@ -121,8 +121,10 @@ func TestStatsGroupOperations(t *testing.T) {
 	})
 
 	testGroupStatsInfo := NewStatsGroup(ctx, "test-group")
-	testGroupStatsInfo.Deletes(1)
-	GlobalStats().Deletes(41)
+	require.NoError(t, testGroupStatsInfo.DeleteFile(ctx, 0))
+	for i := 0; i < 41; i++ {
+		require.NoError(t, GlobalStats().DeleteFile(ctx, 0))
+	}
 
 	t.Run("core/group-list", func(t *testing.T) {
 		call := rc.Calls.Get("core/group-list")
diff --git a/fs/operations/operations.go b/fs/operations/operations.go
index 7288cbf0d..0740a198d 100644
--- a/fs/operations/operations.go
+++ b/fs/operations/operations.go
@@ -632,20 +632,13 @@ func SuffixName(ctx context.Context, remote string) string {
 // If backupDir is set then it moves the file to there instead of
 // deleting
 func DeleteFileWithBackupDir(ctx context.Context, dst fs.Object, backupDir fs.Fs) (err error) {
-	ci := fs.GetConfig(ctx)
 	tr := accounting.Stats(ctx).NewCheckingTransfer(dst, "deleting")
 	defer func() {
 		tr.Done(ctx, err)
 	}()
-	deletesSize := accounting.Stats(ctx).DeletesSize(0) // file not yet deleted, we should not add at this time
-	size := dst.Size()
-	if int64(ci.MaxDeleteSize) != -1 && (deletesSize+size) > int64(ci.MaxDeleteSize) {
-		return fserrors.FatalError(errors.New("--max-delete-size threshold reached"))
-	}
-	_ = accounting.Stats(ctx).DeletesSize(size) // here we count
-	numDeletes := accounting.Stats(ctx).Deletes(1)
-	if ci.MaxDelete != -1 && numDeletes > ci.MaxDelete {
-		return fserrors.FatalError(errors.New("--max-delete threshold reached"))
+	err = accounting.Stats(ctx).DeleteFile(ctx, dst.Size())
+	if err != nil {
+		return err
 	}
 	action, actioned := "delete", "Deleted"
 	if backupDir != nil {
diff --git a/fs/rc/rcserver/rcserver_test.go b/fs/rc/rcserver/rcserver_test.go
index 5a6c9a129..278722297 100644
--- a/fs/rc/rcserver/rcserver_test.go
+++ b/fs/rc/rcserver/rcserver_test.go
@@ -587,7 +587,9 @@ func TestMetrics(t *testing.T) {
 
 	// Test changing a couple options
 	stats.Bytes(500)
-	stats.Deletes(30)
+	for i := 0; i < 30; i++ {
+		require.NoError(t, stats.DeleteFile(context.Background(), 0))
+	}
 	stats.Errors(2)
 	stats.Bytes(324)
 
@@ -619,7 +621,7 @@ func makeMetricsTestCases(stats *accounting.StatsInfo) (tests []testRun) {
 		URL:      "/metrics",
 		Method:   "GET",
 		Status:   http.StatusOK,
-		Contains: regexp.MustCompile(fmt.Sprintf("rclone_files_deleted_total %d", stats.Deletes(0))),
+		Contains: regexp.MustCompile(fmt.Sprintf("rclone_files_deleted_total %d", stats.GetDeletes())),
 	}, {
 		Name:     "Files Transferred Metric",
 		URL:      "/metrics",