chore(model): more efficient tracking of renames during scan (#10653)

lib/model/folder.go

@@ -556,12 +556,14 @@ type scanBatch struct {
 	f           *folder
 	updateBatch *FileInfoBatch
 	toRemove    []string
+	deleted     map[string]struct{}
 }
 
 func (f *folder) newScanBatch() *scanBatch {
 	b := &scanBatch{
 		f:        f,
 		toRemove: make([]string, 0, maxToRemove),
+		deleted:  make(map[string]struct{}),
 	}
 	b.updateBatch = NewFileInfoBatch(func(fs []protocol.FileInfo) error {
 		if err := b.f.getHealthErrorWithoutIgnores(); err != nil {
@@ -569,6 +571,7 @@ func (f *folder) newScanBatch() *scanBatch {
 			return err
 		}
 		b.f.updateLocalsFromScanning(fs)
+		clear(b.deleted)
 		return nil
 	})
 	return b
@@ -604,6 +607,15 @@ func (b *scanBatch) FlushIfFull() error {
 	return b.updateBatch.FlushIfFull()
 }
 
+func (b *scanBatch) markDeleted(name string) {
+	b.deleted[name] = struct{}{}
+}
+
+func (b *scanBatch) hasDeleted(name string) bool {
+	_, ok := b.deleted[name]
+	return ok
+}
+
 // Update adds the fileinfo to the batch for updating, and does a few checks.
 // It returns false if the checks result in the file not going to be updated or removed.
 func (b *scanBatch) Update(fi protocol.FileInfo) (bool, error) {
@@ -680,7 +692,6 @@ func (f *folder) scanSubdirsChangedAndNew(ctx context.Context, subDirs []string,
 		fchan = scanner.Walk(scanCtx, scanConfig)
 	}
 
-	alreadyUsedOrExisting := make(map[string]struct{})
 	for res := range fchan {
 		if res.Err != nil {
 			f.newScanError(res.Path, res.Err)
@@ -705,11 +716,12 @@ func (f *folder) scanSubdirsChangedAndNew(ctx context.Context, subDirs []string,
 		switch f.Type {
 		case config.FolderTypeReceiveOnly, config.FolderTypeReceiveEncrypted:
 		default:
-			if nf, ok := f.findRename(ctx, res.File, alreadyUsedOrExisting); ok {
+			if nf, ok := f.findRename(ctx, res.File, batch); ok {
 				if ok, err := batch.Update(nf); err != nil {
 					return 0, err
 				} else if ok {
 					changes++
+					batch.markDeleted(nf.Name)
 				}
 			}
 		}
@@ -878,7 +890,7 @@ outer:
 	return changes, nil
 }
 
-func (f *folder) findRename(ctx context.Context, file protocol.FileInfo, alreadyUsedOrExisting map[string]struct{}) (protocol.FileInfo, bool) {
+func (f *folder) findRename(ctx context.Context, file protocol.FileInfo, batch *scanBatch) (protocol.FileInfo, bool) {
 	if len(file.Blocks) == 0 || file.Size == 0 {
 		return protocol.FileInfo{}, false
 	}
@@ -899,11 +911,10 @@ loop:
 		}
 
 		if fi.Name == file.Name {
-			alreadyUsedOrExisting[fi.Name] = struct{}{}
 			continue
 		}
 
-		if _, ok := alreadyUsedOrExisting[fi.Name]; ok {
+		if batch.hasDeleted(fi.Name) {
 			continue
 		}
 
@@ -922,8 +933,6 @@ loop:
 			continue
 		}
 
-		alreadyUsedOrExisting[fi.Name] = struct{}{}
-
 		if !osutil.IsDeleted(f.mtimefs, fi.Name) {
 			continue
 		}

lib/model/model_test.go

@@ -3344,6 +3344,87 @@ func TestRenameSameFile(t *testing.T) {
 	}
 }
 
+// TestRenameBatchFlush verifies that rename detection works correctly when
+// a batch flush happens mid-scan. With enough files to exceed
+// MaxBatchSizeFiles the scan batch flushes at least once, clearing the
+// in-memory deleted-tracking map. After the flush the database itself
+// guards against reusing an already-consumed rename source. The fake
+// filesystem iterates in non-deterministic order, so the two rename pairs
+// may or may not straddle a flush boundary on any given run; with
+// 2*MaxBatchSizeFiles filler files the cross-flush case is hit roughly half
+// the time.
+func TestRenameBatchFlush(t *testing.T) {
+	wcfg, fcfg := newDefaultCfgWrapper(t)
+	m := setupModel(t, wcfg)
+	defer cleanupModel(m)
+
+	ffs := fcfg.Filesystem()
+
+	// Two source files with identical content so they share a blocks hash.
+	content := []byte("shared-content-for-rename-detection")
+	writeFile(t, ffs, "src-a", content)
+	writeFile(t, ffs, "src-b", content)
+
+	m.ScanFolders()
+
+	// Delete the sources and create two new destinations with the same
+	// content plus enough filler files to force at least one batch flush.
+	must(t, ffs.Remove("src-a"))
+	must(t, ffs.Remove("src-b"))
+	writeFile(t, ffs, "dst-a", content)
+	writeFile(t, ffs, "dst-b", content)
+	for i := range MaxBatchSizeFiles * 2 {
+		writeFile(t, ffs, fmt.Sprintf("filler-%04d", i), []byte(fmt.Sprintf("filler-%04d", i)))
+	}
+
+	m.ScanFolders()
+
+	// Collect all files keyed by name.
+	files := make(map[string]protocol.FileInfo)
+	it, errFn := m.LocalFilesSequenced("default", protocol.LocalDeviceID, 0)
+	for fi := range it {
+		files[fi.FileName()] = fi
+	}
+	if err := errFn(); err != nil {
+		t.Fatal(err)
+	}
+
+	for _, name := range []string{"src-a", "src-b"} {
+		fi, ok := files[name]
+		if !ok {
+			t.Fatalf("%q not found in DB", name)
+		}
+		if !fi.IsDeleted() {
+			t.Fatalf("%q should be deleted", name)
+		}
+	}
+
+	for _, name := range []string{"dst-a", "dst-b"} {
+		fi, ok := files[name]
+		if !ok {
+			t.Fatalf("%q not found in DB", name)
+		}
+		if fi.IsDeleted() {
+			t.Fatalf("%q should not be deleted", name)
+		}
+	}
+
+	// When rename detection works the deleted source is appended to the
+	// batch right after its destination, so their sequences are adjacent
+	// (src.seq == dst.seq + 1). If detection failed the sources would
+	// only be deleted in a later scan phase with much higher sequences.
+	dstSeqs := map[int64]bool{
+		files["dst-a"].SequenceNo(): true,
+		files["dst-b"].SequenceNo(): true,
+	}
+	for _, name := range []string{"src-a", "src-b"} {
+		srcSeq := files[name].SequenceNo()
+		if !dstSeqs[srcSeq-1] {
+			t.Errorf("deleted %q (seq %d) not adjacent to a destination file; rename was not detected", name, srcSeq)
+		}
+	}
+}
+
 func TestBlockListMap(t *testing.T) {
 	wcfg, fcfg := newDefaultCfgWrapper(t)
 	m := setupModel(t, wcfg)