From 83521923f6be851a79f9f7064214f547ef43c681 Mon Sep 17 00:00:00 2001
From: Vitaliy Filippov <vitalif@yourcmc.ru>
Date: Tue, 3 Sep 2024 11:43:03 +0300
Subject: [PATCH] Fix #98 - conflicts incorrectly detected for renamed files
 leading to updates being thrown away

---
 internal/goofys_test.go | 66 +++++++++++++++++++++++++++++++++++++++++
 internal/handles.go     | 12 ++++++--
 2 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/internal/goofys_test.go b/internal/goofys_test.go
index 7160b6c..e8afe53 100644
--- a/internal/goofys_test.go
+++ b/internal/goofys_test.go
@@ -53,6 +53,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"syscall"
 	"time"
 
@@ -2892,3 +2893,68 @@ func (s *GoofysTest) TestSlurpDisappear(t *C) {
 	s.setS3(nil)
 	s.assertEntries(t, in, nil)
 }
+
+func (s *GoofysTest) TestListBeforeFlushRename(t *C) {
+	s.fs.flags.StatCacheTTL = 1 * time.Second
+	s.fs.flags.ReadRetryAttempts = 3
+
+	root := s.getRoot(t)
+
+	in, err := root.LookUp("file1", false)
+	t.Assert(err, IsNil)
+	fh, err := in.OpenFile()
+	t.Assert(err, IsNil)
+	err = fh.WriteFile(0, []byte("file1 old version"), true)
+	t.Assert(err, IsNil)
+	fh.Release()
+	err = in.SyncFile()
+	t.Assert(err, IsNil)
+
+	in, fh, err = root.Create("file1.new")
+	t.Assert(err, IsNil)
+	err = fh.WriteFile(0, []byte("hello world"), true)
+	t.Assert(err, IsNil)
+	fh.Release()
+	err = in.SyncFile()
+	t.Assert(err, IsNil)
+
+	// Pause flushing and rename file1.new -> file1
+	atomic.AddInt64(&s.fs.activeFlushers, s.fs.flags.MaxFlushers)
+	err = root.Rename("file1.new", root, "file1")
+	t.Assert(err, IsNil)
+
+	// Sleep 1 second to make file1 metadata cache expire and perform a listing
+	// Without renameInProgress in inode.SetFromBlobItem() it was detecting a conflict and
+	// rolling back the file state. Even worse, it looped forever in retryRead() without
+	// ReadRetryAttempts = 3, because it retries io.EOF...
+	time.Sleep(1 * time.Second)
+
+	// Check that file1 isn't rolled back to the old content
+	in, err = root.LookUp("file1", false)
+	t.Assert(err, IsNil)
+	fh, err = in.OpenFile()
+	t.Assert(err, IsNil)
+	bufs, nread, err := fh.ReadFile(0, 4096)
+	t.Assert(len(bufs), Equals, 1)
+	t.Assert(string(bufs[0]), Equals, "hello world")
+	t.Assert(nread, Equals, 11)
+	t.Assert(err, IsNil)
+	fh.Release()
+
+	// Resume flushing
+	atomic.AddInt64(&s.fs.activeFlushers, -s.fs.flags.MaxFlushers)
+	s.fs.WakeupFlusher()
+	err = in.SyncFile()
+	t.Assert(err, IsNil)
+
+	// Check again
+	time.Sleep(1 * time.Second)
+	fh, err = in.OpenFile()
+	t.Assert(err, IsNil)
+	bufs, nread, err = fh.ReadFile(0, 4096)
+	t.Assert(len(bufs), Equals, 1)
+	t.Assert(string(bufs[0]), Equals, "hello world")
+	t.Assert(nread, Equals, 11)
+	t.Assert(err, IsNil)
+	fh.Release()
+}
diff --git a/internal/handles.go b/internal/handles.go
index ef370e8..62748a3 100644
--- a/internal/handles.go
+++ b/internal/handles.go
@@ -195,14 +195,20 @@ func (inode *Inode) SetFromBlobItem(item *BlobItemOutput) {
 	inode.mu.Lock()
 	defer inode.mu.Unlock()
 
-	patchInProgress := inode.fs.flags.UsePatch && inode.mpu == nil && inode.CacheState == ST_MODIFIED && inode.IsFlushing > 0
 	// We always just drop our local cache when inode size or etag changes remotely
 	// It's the simplest method of conflict resolution
 	// Otherwise we may not be able to make a correct object version
-	//
+
 	// If ongoing patch requests exist, then concurrent etag changes is normal. In current implementation
 	// it is hard to reliably distinguish actual data conflicts from concurrent patch updates.
-	if !patchInProgress && (item.ETag != nil && inode.knownETag != *item.ETag || item.Size != inode.knownSize) {
+	patchInProgress := inode.fs.flags.UsePatch && inode.mpu == nil && inode.CacheState == ST_MODIFIED && inode.IsFlushing > 0
+
+	// If a file is renamed from a different file then we also don't know its server-side
+	// ETag or Size for sure, so the simplest fix is to also ignore this check
+	renameInProgress := inode.oldName != ""
+
+	if (item.ETag != nil && inode.knownETag != *item.ETag || item.Size != inode.knownSize) &&
+		!patchInProgress && !renameInProgress {
 		if inode.CacheState != ST_CACHED && (inode.knownETag != "" || inode.knownSize > 0) {
 			s3Log.Warnf("Conflict detected (inode %v): server-side ETag or size of %v"+
 				" (%v, %v) differs from local (%v, %v). File is changed remotely, dropping cache",