lib/db, lib/model: Resolve identical recv only items (fixes #5130) (#5230)

lib/db/set_test.go

@@ -1178,8 +1178,8 @@ func TestReceiveOnlyAccounting(t *testing.T) {
 	if n := s.GlobalSize().Files; n != 3 {
 		t.Fatal("expected 3 global files after local change, not", n)
 	}
-	if n := s.GlobalSize().Bytes; n != 120 {
-		t.Fatal("expected 120 global bytes after local change, not", n)
+	if n := s.GlobalSize().Bytes; n != 30 {
+		t.Fatal("expected 30 global files after local change, not", n)
 	}
 	if n := s.ReceiveOnlyChangedSize().Files; n != 1 {
 		t.Fatal("expected 1 receive only changed file after local change, not", n)
@@ -1271,6 +1271,44 @@ func TestRemoteInvalidNotAccounted(t *testing.T) {
 	}
 }
 
+func TestNeedWithNewerInvalid(t *testing.T) {
+	ldb := db.OpenMemory()
+
+	s := db.NewFileSet("default", fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb)
+
+	rem0ID := remoteDevice0.Short()
+	rem1ID := remoteDevice1.Short()
+
+	// Initial state: file present on rem0 and rem1, but not locally.
+	file := protocol.FileInfo{Name: "foo"}
+	file.Version = file.Version.Update(rem0ID)
+	s.Update(remoteDevice0, fileList{file})
+	s.Update(remoteDevice1, fileList{file})
+
+	need := needList(s, protocol.LocalDeviceID)
+	if len(need) != 1 {
+		t.Fatal("Locally missing file should be needed")
+	}
+	if !need[0].IsEquivalent(file) {
+		t.Fatalf("Got needed file %v, expected %v", need[0], file)
+	}
+
+	// rem1 sends an invalid file with increased version
+	inv := file
+	inv.Version = inv.Version.Update(rem1ID)
+	inv.RawInvalid = true
+	s.Update(remoteDevice1, fileList{inv})
+
+	// We still have an old file, we need the newest valid file
+	need = needList(s, protocol.LocalDeviceID)
+	if len(need) != 1 {
+		t.Fatal("Locally missing file should be needed regardless of invalid files")
+	}
+	if !need[0].IsEquivalent(file) {
+		t.Fatalf("Got needed file %v, expected %v", need[0], file)
+	}
+}
+
 func replace(fs *db.FileSet, device protocol.DeviceID, files []protocol.FileInfo) {
 	fs.Drop(device)
 	fs.Update(device, files)

lib/db/structs.go

@@ -12,6 +12,7 @@ package db
 import (
 	"bytes"
 	"fmt"
+	"sort"
 	"time"
 
 	"github.com/syncthing/syncthing/lib/protocol"
@@ -150,7 +151,7 @@ func (vl VersionList) String() string {
 			b.WriteString(", ")
 		}
 		copy(id[:], v.Device)
-		fmt.Fprintf(&b, "{%v, %v, %v}", v.Version, id, v.Invalid)
+		fmt.Fprintf(&b, "{%v, %v}", v.Version, id)
 	}
 	b.WriteString("}")
 	return b.String()
@@ -175,12 +176,15 @@ func (vl VersionList) update(folder, device []byte, file protocol.FileInfo, db *
 		Version: file.Version,
 		Invalid: file.IsInvalid(),
 	}
-	for i, v := range vl.Versions {
-		switch v.Version.Compare(file.Version) {
+	i := 0
+	if nv.Invalid {
+		i = sort.Search(len(vl.Versions), func(j int) bool {
+			return vl.Versions[j].Invalid
+		})
+	}
+	for ; i < len(vl.Versions); i++ {
+		switch vl.Versions[i].Version.Compare(file.Version) {
 		case protocol.Equal:
-			if nv.Invalid {
-				continue
-			}
 			fallthrough
 
 		case protocol.Lesser:
@@ -198,7 +202,7 @@ func (vl VersionList) update(folder, device []byte, file protocol.FileInfo, db *
 			// to determine the winner.)
 			//
 			// A surprise missing file entry here is counted as a win for us.
-			if of, ok := db.getFile(db.keyer.GenerateDeviceFileKey(nil, folder, v.Device, []byte(file.Name))); !ok || file.WinsConflict(of) {
+			if of, ok := db.getFile(db.keyer.GenerateDeviceFileKey(nil, folder, vl.Versions[i].Device, []byte(file.Name))); !ok || file.WinsConflict(of) {
 				vl = vl.insertAt(i, nv)
 				return vl, removedFV, removedAt, i
 			}

lib/db/transactions.go

@@ -163,7 +163,7 @@ func need(global FileIntf, haveLocal bool, localVersion protocol.Vector) bool {
 		return false
 	}
 	// We don't need the global file if we already have the same version.
-	if haveLocal && localVersion.Equal(global.FileVersion()) {
+	if haveLocal && localVersion.GreaterEqual(global.FileVersion()) {
 		return false
 	}
 	return true

lib/model/folder_recvonly_test.go

@@ -196,8 +196,8 @@ func TestRecvOnlyRevertNeeds(t *testing.T) {
 
 	size = m.GlobalSize("ro")
 	const sizeOfDir = 128
-	if size.Files != 1 || size.Bytes != sizeOfDir+int64(len(newData)) {
-		t.Fatalf("Global: expected the new file to be reflected: %+v", size)
+	if size.Files != 1 || size.Bytes != sizeOfDir+int64(len(oldData)) {
+		t.Fatalf("Global: expected no change due to the new file: %+v", size)
 	}
 	size = m.LocalSize("ro")
 	if size.Files != 1 || size.Bytes != sizeOfDir+int64(len(newData)) {
@@ -230,6 +230,99 @@ func TestRecvOnlyRevertNeeds(t *testing.T) {
 	}
 }
 
+func TestRecvOnlyUndoChanges(t *testing.T) {
+	if err := os.RemoveAll("_recvonly"); err != nil && !os.IsNotExist(err) {
+		t.Fatal(err)
+	}
+	defer func() {
+		if err := os.RemoveAll("_recvonly"); err != nil && !os.IsNotExist(err) {
+			t.Fatal(err)
+		}
+	}()
+
+	// Create some test data
+
+	if err := os.MkdirAll("_recvonly/.stfolder", 0755); err != nil {
+		t.Fatal(err)
+	}
+	oldData := []byte("hello\n")
+	knownFiles := setupKnownFiles(t, oldData)
+
+	// Get us a model up and running
+
+	m := setupROFolder()
+	defer m.Stop()
+
+	m.fmut.Lock()
+	fset := m.folderFiles["ro"]
+	m.fmut.Unlock()
+	folderFs := fset.MtimeFS()
+
+	// Send and index update for the known stuff
+
+	m.Index(device1, "ro", knownFiles)
+	m.updateLocalsFromScanning("ro", knownFiles)
+
+	// Start the folder. This will cause a scan.
+
+	m.StartFolder("ro")
+	m.ScanFolder("ro")
+
+	// Everything should be in sync.
+
+	size := m.GlobalSize("ro")
+	if size.Files != 1 || size.Directories != 1 {
+		t.Fatalf("Global: expected 1 file and 1 directory: %+v", size)
+	}
+	size = m.LocalSize("ro")
+	if size.Files != 1 || size.Directories != 1 {
+		t.Fatalf("Local: expected 1 file and 1 directory: %+v", size)
+	}
+	size = m.NeedSize("ro")
+	if size.Files+size.Directories > 0 {
+		t.Fatalf("Need: expected nothing: %+v", size)
+	}
+	size = m.ReceiveOnlyChangedSize("ro")
+	if size.Files+size.Directories > 0 {
+		t.Fatalf("ROChanged: expected nothing: %+v", size)
+	}
+
+	// Create a file and modify another
+
+	file := "_recvonly/foo"
+	if err := ioutil.WriteFile(file, []byte("hello\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := ioutil.WriteFile("_recvonly/knownDir/knownFile", []byte("bye\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	m.ScanFolder("ro")
+
+	size = m.ReceiveOnlyChangedSize("ro")
+	if size.Files != 2 {
+		t.Fatalf("Receive only: expected 2 files: %+v", size)
+	}
+
+	// Remove the file again and undo the modification
+
+	if err := os.Remove(file); err != nil {
+		t.Fatal(err)
+	}
+	if err := ioutil.WriteFile("_recvonly/knownDir/knownFile", oldData, 0644); err != nil {
+		t.Fatal(err)
+	}
+	folderFs.Chtimes("knownDir/knownFile", knownFiles[1].ModTime(), knownFiles[1].ModTime())
+
+	m.ScanFolder("ro")
+
+	size = m.ReceiveOnlyChangedSize("ro")
+	if size.Files+size.Directories+size.Deleted != 0 {
+		t.Fatalf("Receive only: expected all zero: %+v", size)
+	}
+}
+
 func setupKnownFiles(t *testing.T, data []byte) []protocol.FileInfo {
 	if err := os.MkdirAll("_recvonly/knownDir", 0755); err != nil {
 		t.Fatal(err)

lib/model/model.go

@@ -2058,14 +2058,38 @@ func (m *Model) internalScanFolderSubdirs(ctx context.Context, folder string, su
 		return err
 	}
 
-	batch := newFileInfoBatch(func(fs []protocol.FileInfo) error {
+	batchFn := func(fs []protocol.FileInfo) error {
 		if err := runner.CheckHealth(); err != nil {
 			l.Debugf("Stopping scan of folder %s due to: %s", folderCfg.Description(), err)
 			return err
 		}
 		m.updateLocalsFromScanning(folder, fs)
 		return nil
-	})
+	}
+	// Resolve items which are identical with the global state.
+	if localFlags&protocol.FlagLocalReceiveOnly != 0 {
+		oldBatchFn := batchFn // can't reference batchFn directly (recursion)
+		batchFn = func(fs []protocol.FileInfo) error {
+			for i := range fs {
+				switch gf, ok := fset.GetGlobal(fs[i].Name); {
+				case !ok:
+					continue
+				case gf.IsEquivalentOptional(fs[i], false, false, protocol.FlagLocalReceiveOnly):
+					// What we have locally is equivalent to the global file.
+					fs[i].Version = fs[i].Version.Merge(gf.Version)
+					fallthrough
+				case fs[i].IsDeleted() && gf.IsReceiveOnlyChanged():
+					// Our item is deleted and the global item is our own
+					// receive only file. We can't delete file infos, so
+					// we just pretend it is a normal deleted file (nobody
+					// cares about that).
+					fs[i].LocalFlags &^= protocol.FlagLocalReceiveOnly
+				}
+			}
+			return oldBatchFn(fs)
+		}
+	}
+	batch := newFileInfoBatch(batchFn)
 
 	// Schedule a pull after scanning, but only if we actually detected any
 	// changes.