vfs: fix duplicates on rename - fixes #5469

Before this change, if there was an existing file being uploaded when
a file was renamed on top of it, then both would be uploaded. This
causes a duplicate in Google Drive as both files get uploaded at the
same time. This was triggered reliably by LibreOffice saving doc
files.

This fix removes any duplicates in the upload queue on rename.

vfs/vfscache/writeback/writeback.go

@@ -276,13 +276,12 @@ func (wb *WriteBack) Add(id Handle, name string, modified bool, putFn PutFn) Han
 	return wbItem.id
 }
 
-// Remove should be called when a file should be removed from the
+// _remove should be called when a file should be removed from the
 // writeback queue. This cancels a writeback if there is one and
 // doesn't return the item to the queue.
-func (wb *WriteBack) Remove(id Handle) (found bool) {
+//
-	wb.mu.Lock()
+// This should be called with the lock held
-	defer wb.mu.Unlock()
+func (wb *WriteBack) _remove(id Handle) (found bool) {
-
 	wbItem, found := wb.lookup[id]
 	if found {
 		fs.Debugf(wbItem.name, "vfs cache: cancelling writeback (uploading %v) %p item %d", wbItem.uploading, wbItem, wbItem.id)
@@ -299,6 +298,16 @@ func (wb *WriteBack) Remove(id Handle) (found bool) {
 	return found
 }
 
+// Remove should be called when a file should be removed from the
+// writeback queue. This cancels a writeback if there is one and
+// doesn't return the item to the queue.
+func (wb *WriteBack) Remove(id Handle) (found bool) {
+	wb.mu.Lock()
+	defer wb.mu.Unlock()
+
+	return wb._remove(id)
+}
+
 // Rename should be called when a file might be uploading and it gains
 // a new name. This will cancel the upload and put it back in the
 // queue.
@@ -314,6 +323,15 @@ func (wb *WriteBack) Rename(id Handle, name string) {
 		// We are uploading already so cancel the upload
 		wb._cancelUpload(wbItem)
 	}
+
+	// Check to see if there are any uploads with the existing
+	// name and remove them
+	for existingID, existingItem := range wb.lookup {
+		if existingID != id && existingItem.name == name {
+			wb._remove(existingID)
+		}
+	}
+
 	wbItem.name = name
 	// Kick the timer on
 	wb.items._update(wbItem, wb._newExpiry())

vfs/vfscache/writeback/writeback_test.go

@@ -585,6 +585,53 @@ func TestWriteBackRename(t *testing.T) {
 	assert.Equal(t, wbItem.name, "three")
 }
 
+// TestWriteBackRenameDuplicates checks that if we rename an entry and
+// make a duplicate, we remove the duplicate.
+func TestWriteBackRenameDuplicates(t *testing.T) {
+	wb, cancel := newTestWriteBack(t)
+	defer cancel()
+
+	// add item "one"
+	pi1 := newPutItem(t)
+	id1 := wb.Add(0, "one", true, pi1.put)
+	wbItem1 := wb.lookup[id1]
+	checkOnHeap(t, wb, wbItem1)
+	checkInLookup(t, wb, wbItem1)
+	assert.Equal(t, wbItem1.name, "one")
+
+	<-pi1.started
+	checkNotOnHeap(t, wb, wbItem1)
+	checkInLookup(t, wb, wbItem1)
+
+	// add item "two"
+	pi2 := newPutItem(t)
+	id2 := wb.Add(0, "two", true, pi2.put)
+	wbItem2 := wb.lookup[id2]
+	checkOnHeap(t, wb, wbItem2)
+	checkInLookup(t, wb, wbItem2)
+	assert.Equal(t, wbItem2.name, "two")
+
+	<-pi2.started
+	checkNotOnHeap(t, wb, wbItem2)
+	checkInLookup(t, wb, wbItem2)
+
+	// rename "two" to "one"
+	wb.Rename(id2, "one")
+
+	// check "one" is cancelled and removed from heap and lookup
+	checkNotOnHeap(t, wb, wbItem1)
+	checkNotInLookup(t, wb, wbItem1)
+	assert.True(t, pi1.cancelled)
+	assert.Equal(t, wbItem1.name, "one")
+
+	// check "two" (now called "one"!) has been cancelled and will
+	// be retried
+	checkOnHeap(t, wb, wbItem2)
+	checkInLookup(t, wb, wbItem2)
+	assert.True(t, pi2.cancelled)
+	assert.Equal(t, wbItem2.name, "one")
+}
+
 func TestWriteBackCancelUpload(t *testing.T) {
 	wb, cancel := newTestWriteBack(t)
 	defer cancel()