From 4680c0776d4219c82e8362ae114423f6ff20c2df Mon Sep 17 00:00:00 2001
From: Ivan Andreev <ivandeex@gmail.com>
Date: Wed, 7 Jul 2021 16:50:19 +0300
Subject: [PATCH] backend/local: skip entries removed concurrently with List()
 (#5297)

This change fixes the bug described below:
if a file is removed while the local backend List() runs,
the call will flag an accounting error.
The bug manifests itself if local backend is the Sync target
due to intrinsic concurrency.
The odds to hit this bug depend on --checkers and --transfers.
Chunker over local backend is affected even more because
updating a composite object with a smaller size content
translates into removing chunks on the underlying file system
and involves a number of List() calls.
---
 backend/local/local.go |  4 ++++
 fs/sync/sync.go        |  4 +---
 fs/sync/sync_test.go   | 51 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/backend/local/local.go b/backend/local/local.go
index 01fed30a3..85b167442 100644
--- a/backend/local/local.go
+++ b/backend/local/local.go
@@ -467,6 +467,10 @@ func (f *Fs) List(ctx context.Context, dir string) (entries fs.DirEntries, err e
 				for _, name := range names {
 					namepath := filepath.Join(fsDirPath, name)
 					fi, fierr := os.Lstat(namepath)
+					if os.IsNotExist(fierr) {
+						// skip entry removed by a concurrent goroutine
+						continue
+					}
 					if fierr != nil {
 						err = errors.Wrapf(err, "failed to read directory %q", namepath)
 						fs.Errorf(dir, "%v", fierr)
diff --git a/fs/sync/sync.go b/fs/sync/sync.go
index 4958479f7..7ac9a8a59 100644
--- a/fs/sync/sync.go
+++ b/fs/sync/sync.go
@@ -624,9 +624,7 @@ func (s *syncCopyMove) srcParentDirCheck(entry fs.DirEntry) {
 	if parentDir == "." {
 		parentDir = ""
 	}
-	if _, ok := s.srcEmptyDirs[parentDir]; ok {
-		delete(s.srcEmptyDirs, parentDir)
-	}
+	delete(s.srcEmptyDirs, parentDir)
 }
 
 // parseTrackRenamesStrategy turns a config string into a trackRenamesStrategy
diff --git a/fs/sync/sync_test.go b/fs/sync/sync_test.go
index a2d2933bb..1b48c2019 100644
--- a/fs/sync/sync_test.go
+++ b/fs/sync/sync_test.go
@@ -1973,3 +1973,54 @@ func TestMaxTransfer(t *testing.T) {
 	t.Run("Soft", func(t *testing.T) { test(t, fs.CutoffModeSoft) })
 	t.Run("Cautious", func(t *testing.T) { test(t, fs.CutoffModeCautious) })
 }
+
+func testSyncConcurrent(t *testing.T, subtest string) {
+	const (
+		NFILES     = 20
+		NCHECKERS  = 4
+		NTRANSFERS = 4
+	)
+
+	ctx, ci := fs.AddConfig(context.Background())
+	ci.Checkers = NCHECKERS
+	ci.Transfers = NTRANSFERS
+
+	r := fstest.NewRun(t)
+	defer r.Finalise()
+	stats := accounting.GlobalStats()
+
+	itemsBefore := []fstest.Item{}
+	itemsAfter := []fstest.Item{}
+	for i := 0; i < NFILES; i++ {
+		nameBoth := fmt.Sprintf("both%d", i)
+		nameOnly := fmt.Sprintf("only%d", i)
+		switch subtest {
+		case "delete":
+			fileBoth := r.WriteBoth(ctx, nameBoth, "potato", t1)
+			fileOnly := r.WriteObject(ctx, nameOnly, "potato", t1)
+			itemsBefore = append(itemsBefore, fileBoth, fileOnly)
+			itemsAfter = append(itemsAfter, fileBoth)
+		case "truncate":
+			fileBoth := r.WriteBoth(ctx, nameBoth, "potato", t1)
+			fileFull := r.WriteObject(ctx, nameOnly, "potato", t1)
+			fileEmpty := r.WriteFile(nameOnly, "", t1)
+			itemsBefore = append(itemsBefore, fileBoth, fileFull)
+			itemsAfter = append(itemsAfter, fileBoth, fileEmpty)
+		}
+	}
+
+	fstest.CheckItems(t, r.Fremote, itemsBefore...)
+	stats.ResetErrors()
+	err := Sync(ctx, r.Fremote, r.Flocal, false)
+	assert.NoError(t, err, "Sync must not return a error")
+	assert.False(t, stats.Errored(), "Low level errors must not have happened")
+	fstest.CheckItems(t, r.Fremote, itemsAfter...)
+}
+
+func TestSyncConcurrentDelete(t *testing.T) {
+	testSyncConcurrent(t, "delete")
+}
+
+func TestSyncConcurrentTruncate(t *testing.T) {
+	testSyncConcurrent(t, "truncate")
+}