dedupe: fix rename mode giving up after 100 names and make it faster - fixes #9860

Before this change `rclone dedupe --dedupe-mode rename` probed the
backend for each candidate `name-N.ext` in turn and gave up when it
had tried 100 names for a given object. With daily runs against the
same duplicated filename this ceiling was eventually reached and
rclone logged "Could not find an available new name". Each probe was
also a backend lookup, so a run against 99 existing names took
minutes on Google Drive.

The rename now uses the listing dedupe has already made to skip names
known to be taken without asking the backend, and only confirms the
final candidate with NewObject (the listing may be incomplete because
of filters). The suffix counter is shared between the objects being
renamed so no name is checked twice. The safety limit is raised to
10000 which, thanks to the listing, no longer costs a lookup per name.

fs/operations/dedupe.go

@@ -16,8 +16,17 @@ import (
 	"github.com/rclone/rclone/fs/walk"
 )
 
+// maxDedupeSuffix is the largest -N suffix dedupeRename will try
+// before giving up on renaming an object.
+const maxDedupeSuffix = 10000
+
 // dedupeRename renames the objs slice to different names
-func dedupeRename(ctx context.Context, f fs.Fs, remote string, objs []fs.Object) {
+//
+// files is the listing of f keyed by remote. Names found in it are
+// skipped without asking the backend, and the names allocated here
+// are added to it. Candidates not in the listing are still checked
+// with NewObject as the listing may be incomplete due to filters.
+func dedupeRename(ctx context.Context, f fs.Fs, remote string, objs []fs.Object, files map[string][]fs.Object) {
 	doMove := f.Features().Move
 	if doMove == nil {
 		fs.Fatalf(nil, "Fs %v doesn't support Move", f)
@@ -25,23 +34,32 @@ func dedupeRename(ctx context.Context, f fs.Fs, remote string, objs []fs.Object)
 	ext := path.Ext(remote)
 	base := remote[:len(remote)-len(ext)]
 
+	// The suffix is shared between the objects so names known to
+	// be taken are never re-checked.
+	suffix := 0
 outer:
-	for i, o := range objs {
+	for _, o := range objs {
-		suffix := 1
+		var newName string
-		newName := fmt.Sprintf("%s-%d%s", base, i+suffix, ext)
+		for {
-		_, err := f.NewObject(ctx, newName)
+			suffix++
-		for ; err != fs.ErrorObjectNotFound; suffix++ {
+			if suffix > maxDedupeSuffix {
+				fs.Errorf(o, "Could not find an available new name")
+				continue outer
+			}
+			newName = fmt.Sprintf("%s-%d%s", base, suffix, ext)
+			if _, found := files[newName]; found {
+				continue
+			}
+			existing, err := f.NewObject(ctx, newName)
+			if err == fs.ErrorObjectNotFound {
+				break
+			}
 			if err != nil {
 				err = fs.CountError(ctx, err)
 				fs.Errorf(o, "Failed to check for existing object: %v", err)
 				continue outer
 			}
-			if suffix > 100 {
+			files[newName] = append(files[newName], existing)
-				fs.Errorf(o, "Could not find an available new name")
-				continue outer
-			}
-			newName = fmt.Sprintf("%s-%d%s", base, i+suffix, ext)
-			_, err = f.NewObject(ctx, newName)
 		}
 		if !SkipDestructive(ctx, o, "rename") {
 			newObj, err := doMove(ctx, o, newName)
@@ -51,6 +69,7 @@ outer:
 				continue
 			}
 			fs.Infof(newObj, "renamed from: %v", o)
+			files[newName] = append(files[newName], newObj)
 		}
 	}
 }
@@ -158,7 +177,7 @@ func dedupeList(ctx context.Context, f fs.Fs, ht hash.Type, remote string, objs
 }
 
 // dedupeInteractive interactively dedupes the slice of objects
-func dedupeInteractive(ctx context.Context, f fs.Fs, ht hash.Type, remote string, objs []fs.Object, byHash bool) bool {
+func dedupeInteractive(ctx context.Context, f fs.Fs, ht hash.Type, remote string, objs []fs.Object, byHash bool, files map[string][]fs.Object) bool {
 	dedupeList(ctx, f, ht, remote, objs, byHash)
 	commands := []string{"sSkip and do nothing", "kKeep just one (choose which in next step)"}
 	if !byHash {
@@ -171,7 +190,7 @@ func dedupeInteractive(ctx context.Context, f fs.Fs, ht hash.Type, remote string
 		keep := config.ChooseNumber("Enter the number of the file to keep", 1, len(objs))
 		dedupeDeleteAllButOne(ctx, keep-1, remote, objs)
 	case 'r':
-		dedupeRename(ctx, f, remote, objs)
+		dedupeRename(ctx, f, remote, objs, files)
 	case 'q':
 		return false
 	}
@@ -461,6 +480,9 @@ func Deduplicate(ctx context.Context, f fs.Fs, mode DeduplicateMode, byHash bool
 		return err
 	}
 
+	// Renaming adds entries to files while it is being iterated. Go
+	// may or may not visit those, but they only ever hold a single
+	// object so are skipped either way.
 	for remote, objs := range files {
 		if len(objs) <= 1 {
 			continue
@@ -475,7 +497,7 @@ func Deduplicate(ctx context.Context, f fs.Fs, mode DeduplicateMode, byHash bool
 		}
 		switch mode {
 		case DeduplicateInteractive:
-			if !dedupeInteractive(ctx, f, ht, remote, objs, byHash) {
+			if !dedupeInteractive(ctx, f, ht, remote, objs, byHash, files) {
 				return nil
 			}
 		case DeduplicateFirst:
@@ -487,7 +509,7 @@ func Deduplicate(ctx context.Context, f fs.Fs, mode DeduplicateMode, byHash bool
 			sortOldestFirst(objs)
 			dedupeDeleteAllButOne(ctx, 0, remote, objs)
 		case DeduplicateRename:
-			dedupeRename(ctx, f, remote, objs)
+			dedupeRename(ctx, f, remote, objs, files)
 		case DeduplicateLargest:
 			sortSmallestFirst(objs)
 			dedupeDeleteAllButOne(ctx, len(objs)-1, remote, objs)

fs/operations/dedupe_test.go

@@ -2,6 +2,7 @@ package operations_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -246,6 +247,43 @@ func TestDeduplicateRename(t *testing.T) {
 	}))
 }
 
+// Check rename finds a free name when many numbered names already exist
+func TestDeduplicateRenameManyExisting(t *testing.T) {
+	r := fstest.NewRun(t)
+	skipIfCantDedupe(t, r.Fremote)
+
+	// Fill in one-1.txt to one-105.txt so the search has to go past 100
+	const existing = 105
+	var items []fstest.Item
+	for i := 1; i <= existing; i++ {
+		items = append(items, r.WriteObject(context.Background(), fmt.Sprintf("one-%d.txt", i), "This is not a duplicate", t1))
+	}
+	file1 := r.WriteUncheckedObject(context.Background(), "one.txt", "This is one", t1)
+	file2 := r.WriteUncheckedObject(context.Background(), "one.txt", "This is one too", t2)
+	items = append(items, file1, file2)
+	r.CheckWithDuplicates(t, items...)
+
+	err := operations.Deduplicate(context.Background(), r.Fremote, operations.DeduplicateRename, false)
+	require.NoError(t, err)
+
+	// The duplicates are renamed in listing order which isn't
+	// defined, so accept either assignment of the two new names
+	sizes := map[string]int64{}
+	require.NoError(t, walk.ListR(context.Background(), r.Fremote, "", true, -1, walk.ListObjects, func(entries fs.DirEntries) error {
+		entries.ForObject(func(o fs.Object) {
+			sizes[o.Remote()] = o.Size()
+		})
+		return nil
+	}))
+	assert.Equal(t, existing+2, len(sizes))
+	for i := 1; i <= existing; i++ {
+		assert.Equal(t, items[i-1].Size, sizes[fmt.Sprintf("one-%d.txt", i)])
+	}
+	size1 := sizes[fmt.Sprintf("one-%d.txt", existing+1)]
+	size2 := sizes[fmt.Sprintf("one-%d.txt", existing+2)]
+	assert.ElementsMatch(t, []int64{file1.Size, file2.Size}, []int64{size1, size2})
+}
+
 // This should really be a unit test, but the test framework there
 // doesn't have enough tools to make it easy
 func TestMergeDirs(t *testing.T) {