lib/db: Fix prefixed walks (fixes #4925) (#4940)

lib/db/benchmark_test.go

@@ -19,9 +19,13 @@ import (
 )
 
 var files, oneFile, firstHalf, secondHalf []protocol.FileInfo
-var s *db.FileSet
+var benchS *db.FileSet
+
+func lazyInitBenchFileSet() {
+	if benchS != nil {
+		return
+	}
 
-func init() {
 	for i := 0; i < 1000; i++ {
 		files = append(files, protocol.FileInfo{
 			Name:    fmt.Sprintf("file%d", i),
@@ -36,9 +40,9 @@ func init() {
 	oneFile = firstHalf[middle-1 : middle]
 
 	ldb, _ := tempDB()
-	s = db.NewFileSet("test)", fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb)
-	replace(s, remoteDevice0, files)
-	replace(s, protocol.LocalDeviceID, firstHalf)
+	benchS = db.NewFileSet("test)", fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb)
+	replace(benchS, remoteDevice0, files)
+	replace(benchS, protocol.LocalDeviceID, firstHalf)
 }
 
 func tempDB() (*db.Instance, string) {
@@ -70,16 +74,19 @@ func BenchmarkReplaceAll(b *testing.B) {
 }
 
 func BenchmarkUpdateOneChanged(b *testing.B) {
+	lazyInitBenchFileSet()
+
 	changed := make([]protocol.FileInfo, 1)
 	changed[0] = oneFile[0]
 	changed[0].Version = changed[0].Version.Update(myID)
 	changed[0].Blocks = genBlocks(len(changed[0].Blocks))
 
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		if i%1 == 0 {
-			s.Update(protocol.LocalDeviceID, changed)
+			benchS.Update(protocol.LocalDeviceID, changed)
 		} else {
-			s.Update(protocol.LocalDeviceID, oneFile)
+			benchS.Update(protocol.LocalDeviceID, oneFile)
 		}
 	}
 
@@ -87,17 +94,23 @@ func BenchmarkUpdateOneChanged(b *testing.B) {
 }
 
 func BenchmarkUpdateOneUnchanged(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
-		s.Update(protocol.LocalDeviceID, oneFile)
+		benchS.Update(protocol.LocalDeviceID, oneFile)
 	}
 
 	b.ReportAllocs()
 }
 
 func BenchmarkNeedHalf(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		count := 0
-		s.WithNeed(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
+		benchS.WithNeed(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
 			count++
 			return true
 		})
@@ -110,9 +123,12 @@ func BenchmarkNeedHalf(b *testing.B) {
 }
 
 func BenchmarkHave(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		count := 0
-		s.WithHave(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
+		benchS.WithHave(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
 			count++
 			return true
 		})
@@ -125,9 +141,12 @@ func BenchmarkHave(b *testing.B) {
 }
 
 func BenchmarkGlobal(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		count := 0
-		s.WithGlobal(func(fi db.FileIntf) bool {
+		benchS.WithGlobal(func(fi db.FileIntf) bool {
 			count++
 			return true
 		})
@@ -140,9 +159,12 @@ func BenchmarkGlobal(b *testing.B) {
 }
 
 func BenchmarkNeedHalfTruncated(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		count := 0
-		s.WithNeedTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
+		benchS.WithNeedTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
 			count++
 			return true
 		})
@@ -155,9 +177,12 @@ func BenchmarkNeedHalfTruncated(b *testing.B) {
 }
 
 func BenchmarkHaveTruncated(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		count := 0
-		s.WithHaveTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
+		benchS.WithHaveTruncated(protocol.LocalDeviceID, func(fi db.FileIntf) bool {
 			count++
 			return true
 		})
@@ -170,9 +195,12 @@ func BenchmarkHaveTruncated(b *testing.B) {
 }
 
 func BenchmarkGlobalTruncated(b *testing.B) {
+	lazyInitBenchFileSet()
+
+	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		count := 0
-		s.WithGlobalTruncated(func(fi db.FileIntf) bool {
+		benchS.WithGlobalTruncated(func(fi db.FileIntf) bool {
 			count++
 			return true
 		})

lib/db/leveldb_dbinstance.go

@@ -186,20 +186,28 @@ func (db *Instance) removeSequences(folder []byte, fs []protocol.FileInfo) {
 }
 
 func (db *Instance) withHave(folder, device, prefix []byte, truncate bool, fn Iterator) {
+	if len(prefix) > 0 {
+		unslashedPrefix := prefix
+		if bytes.HasSuffix(prefix, []byte{'/'}) {
+			unslashedPrefix = unslashedPrefix[:len(unslashedPrefix)-1]
+		} else {
+			prefix = append(prefix, '/')
+		}
+
+		if f, ok := db.getFileTrunc(db.deviceKey(folder, device, unslashedPrefix), true); ok && !fn(f) {
+			return
+		}
+	}
+
 	t := db.newReadOnlyTransaction()
 	defer t.close()
 
 	dbi := t.NewIterator(util.BytesPrefix(db.deviceKey(folder, device, prefix)[:keyPrefixLen+keyFolderLen+keyDeviceLen+len(prefix)]), nil)
 	defer dbi.Release()
 
-	slashedPrefix := prefix
-	if !bytes.HasSuffix(prefix, []byte{'/'}) {
-		slashedPrefix = append(slashedPrefix, '/')
-	}
-
 	for dbi.Next() {
 		name := db.deviceKeyName(dbi.Key())
-		if len(prefix) > 0 && !bytes.Equal(name, prefix) && !bytes.HasPrefix(name, slashedPrefix) {
+		if len(prefix) > 0 && !bytes.HasPrefix(name, prefix) {
 			return
 		}
 
@@ -277,20 +285,26 @@ func (db *Instance) withAllFolderTruncated(folder []byte, fn func(device []byte,
 }
 
 func (db *Instance) getFile(key []byte) (protocol.FileInfo, bool) {
+	if f, ok := db.getFileTrunc(key, false); ok {
+		return f.(protocol.FileInfo), true
+	}
+	return protocol.FileInfo{}, false
+}
+
+func (db *Instance) getFileTrunc(key []byte, trunc bool) (FileIntf, bool) {
 	bs, err := db.Get(key, nil)
 	if err == leveldb.ErrNotFound {
-		return protocol.FileInfo{}, false
+		return nil, false
 	}
 	if err != nil {
 		l.Debugln("surprise error:", err)
-		return protocol.FileInfo{}, false
+		return nil, false
 	}
 
-	var f protocol.FileInfo
-	err = f.Unmarshal(bs)
+	f, err := unmarshalTrunc(bs, trunc)
 	if err != nil {
 		l.Debugln("unmarshal error:", err)
-		return protocol.FileInfo{}, false
+		return nil, false
 	}
 	return f, true
 }
@@ -306,75 +320,54 @@ func (db *Instance) getGlobal(folder, file []byte, truncate bool) (FileIntf, boo
 		return nil, false
 	}
 
-	var vl VersionList
-	err = vl.Unmarshal(bs)
-	if err == leveldb.ErrNotFound {
-		return nil, false
-	}
-	if err != nil {
-		l.Debugln("unmarshal error:", k, err)
-		return nil, false
-	}
-	if len(vl.Versions) == 0 {
-		l.Debugln("no versions:", k)
+	vl, ok := unmarshalVersionList(bs)
+	if !ok {
 		return nil, false
 	}
 
-	k = db.deviceKey(folder, vl.Versions[0].Device, file)
-	bs, err = t.Get(k, nil)
-	if err != nil {
-		l.Debugln("surprise error:", k, err)
-		return nil, false
+	if fi, ok := db.getFileTrunc(db.deviceKey(folder, vl.Versions[0].Device, file), truncate); ok {
+		return fi, true
 	}
 
-	fi, err := unmarshalTrunc(bs, truncate)
-	if err != nil {
-		l.Debugln("unmarshal error:", k, err)
-		return nil, false
-	}
-	return fi, true
+	return nil, false
 }
 
 func (db *Instance) withGlobal(folder, prefix []byte, truncate bool, fn Iterator) {
+	if len(prefix) > 0 {
+		unslashedPrefix := prefix
+		if bytes.HasSuffix(prefix, []byte{'/'}) {
+			unslashedPrefix = unslashedPrefix[:len(unslashedPrefix)-1]
+		} else {
+			prefix = append(prefix, '/')
+		}
+
+		if f, ok := db.getGlobal(folder, unslashedPrefix, truncate); ok && !fn(f) {
+			return
+		}
+	}
+
 	t := db.newReadOnlyTransaction()
 	defer t.close()
 
 	dbi := t.NewIterator(util.BytesPrefix(db.globalKey(folder, prefix)), nil)
 	defer dbi.Release()
 
-	slashedPrefix := prefix
-	if !bytes.HasSuffix(prefix, []byte{'/'}) {
-		slashedPrefix = append(slashedPrefix, '/')
-	}
-
 	var fk []byte
 	for dbi.Next() {
-		var vl VersionList
-		err := vl.Unmarshal(dbi.Value())
-		if err != nil {
-			l.Debugln("unmarshal error:", err)
-			continue
-		}
-		if len(vl.Versions) == 0 {
-			l.Debugln("no versions:", dbi.Key())
-			continue
-		}
-
 		name := db.globalKeyName(dbi.Key())
-		if len(prefix) > 0 && !bytes.Equal(name, prefix) && !bytes.HasPrefix(name, slashedPrefix) {
+		if len(prefix) > 0 && !bytes.HasPrefix(name, prefix) {
 			return
 		}
 
-		fk = db.deviceKeyInto(fk, folder, vl.Versions[0].Device, name)
-		bs, err := t.Get(fk, nil)
-		if err != nil {
-			l.Debugln("surprise error:", err)
+		vl, ok := unmarshalVersionList(dbi.Value())
+		if !ok {
 			continue
 		}
 
-		f, err := unmarshalTrunc(bs, truncate)
-		if err != nil {
-			l.Debugln("unmarshal error:", err)
+		fk = db.deviceKeyInto(fk, folder, vl.Versions[0].Device, name)
+
+		f, ok := db.getFileTrunc(fk, truncate)
+		if !ok {
 			continue
 		}
 
@@ -395,10 +388,8 @@ func (db *Instance) availability(folder, file []byte) []protocol.DeviceID {
 		return nil
 	}
 
-	var vl VersionList
-	err = vl.Unmarshal(bs)
-	if err != nil {
-		l.Debugln("unmarshal error:", err)
+	vl, ok := unmarshalVersionList(bs)
+	if !ok {
 		return nil
 	}
 
@@ -426,14 +417,8 @@ func (db *Instance) withNeed(folder, device []byte, truncate bool, fn Iterator)
 
 	var fk []byte
 	for dbi.Next() {
-		var vl VersionList
-		err := vl.Unmarshal(dbi.Value())
-		if err != nil {
-			l.Debugln("unmarshal error:", err)
-			continue
-		}
-		if len(vl.Versions) == 0 {
-			l.Debugln("no versions:", dbi.Key())
+		vl, ok := unmarshalVersionList(dbi.Value())
+		if !ok {
 			continue
 		}
 
@@ -586,11 +571,8 @@ func (db *Instance) checkGlobals(folder []byte, meta *metadataTracker) {
 
 	var fk []byte
 	for dbi.Next() {
-		gk := dbi.Key()
-		var vl VersionList
-		err := vl.Unmarshal(dbi.Value())
-		if err != nil {
-			l.Debugln("unmarshal error:", err)
+		vl, ok := unmarshalVersionList(dbi.Value())
+		if !ok {
 			continue
 		}
 
@@ -599,7 +581,7 @@ func (db *Instance) checkGlobals(folder []byte, meta *metadataTracker) {
 		// there are global entries pointing to no longer existing files. Here
 		// we find those and clear them out.
 
-		name := db.globalKeyName(gk)
+		name := db.globalKeyName(dbi.Key())
 		var newVL VersionList
 		for i, version := range vl.Versions {
 			fk = db.deviceKeyInto(fk, folder, version.Device, name)
@@ -923,6 +905,19 @@ func unmarshalTrunc(bs []byte, truncate bool) (FileIntf, error) {
 	return tf, err
 }
 
+func unmarshalVersionList(data []byte) (VersionList, bool) {
+	var vl VersionList
+	if err := vl.Unmarshal(data); err != nil {
+		l.Debugln("unmarshal error:", err)
+		return VersionList{}, false
+	}
+	if len(vl.Versions) == 0 {
+		l.Debugln("empty version list")
+		return VersionList{}, false
+	}
+	return vl, true
+}
+
 // A "better" version of leveldb's errors.IsCorrupted.
 func leveldbIsCorrupted(err error) bool {
 	switch {

lib/db/set.go

@@ -193,8 +193,10 @@ func (s *FileSet) WithHaveSequence(startSeq int64, fn Iterator) {
 	s.db.withHaveSequence([]byte(s.folder), startSeq, nativeFileIterator(fn))
 }
 
+// Except for an item with a path equal to prefix, only children of prefix are iterated.
+// E.g. for prefix "dir", "dir/file" is iterated, but "dir.file" is not.
 func (s *FileSet) WithPrefixedHaveTruncated(device protocol.DeviceID, prefix string, fn Iterator) {
-	l.Debugf("%s WithPrefixedHaveTruncated(%v)", s.folder, device)
+	l.Debugf(`%s WithPrefixedHaveTruncated(%v, "%v")`, s.folder, device, prefix)
 	s.db.withHave([]byte(s.folder), device[:], []byte(osutil.NormalizedFilename(prefix)), true, nativeFileIterator(fn))
 }
 func (s *FileSet) WithGlobal(fn Iterator) {
@@ -207,8 +209,10 @@ func (s *FileSet) WithGlobalTruncated(fn Iterator) {
 	s.db.withGlobal([]byte(s.folder), nil, true, nativeFileIterator(fn))
 }
 
+// Except for an item with a path equal to prefix, only children of prefix are iterated.
+// E.g. for prefix "dir", "dir/file" is iterated, but "dir.file" is not.
 func (s *FileSet) WithPrefixedGlobalTruncated(prefix string, fn Iterator) {
-	l.Debugf("%s WithPrefixedGlobalTruncated()", s.folder, prefix)
+	l.Debugf(`%s WithPrefixedGlobalTruncated("%v")`, s.folder, prefix)
 	s.db.withGlobal([]byte(s.folder), []byte(osutil.NormalizedFilename(prefix)), true, nativeFileIterator(fn))
 }
 

lib/db/set_test.go

@@ -50,6 +50,15 @@ func globalList(s *db.FileSet) []protocol.FileInfo {
 	})
 	return fs
 }
+func globalListPrefixed(s *db.FileSet, prefix string) []db.FileInfoTruncated {
+	var fs []db.FileInfoTruncated
+	s.WithPrefixedGlobalTruncated(prefix, func(fi db.FileIntf) bool {
+		f := fi.(db.FileInfoTruncated)
+		fs = append(fs, f)
+		return true
+	})
+	return fs
+}
 
 func haveList(s *db.FileSet, n protocol.DeviceID) []protocol.FileInfo {
 	var fs []protocol.FileInfo
@@ -61,6 +70,16 @@ func haveList(s *db.FileSet, n protocol.DeviceID) []protocol.FileInfo {
 	return fs
 }
 
+func haveListPrefixed(s *db.FileSet, n protocol.DeviceID, prefix string) []db.FileInfoTruncated {
+	var fs []db.FileInfoTruncated
+	s.WithPrefixedHaveTruncated(n, prefix, func(fi db.FileIntf) bool {
+		f := fi.(db.FileInfoTruncated)
+		fs = append(fs, f)
+		return true
+	})
+	return fs
+}
+
 func needList(s *db.FileSet, n protocol.DeviceID) []protocol.FileInfo {
 	var fs []protocol.FileInfo
 	s.WithNeed(n, func(fi db.FileIntf) bool {
@@ -892,6 +911,32 @@ func TestWithHaveSequence(t *testing.T) {
 	})
 }
 
+func TestIssue4925(t *testing.T) {
+	ldb := db.OpenMemory()
+
+	folder := "test)"
+	s := db.NewFileSet(folder, fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb)
+
+	localHave := fileList{
+		protocol.FileInfo{Name: "dir"},
+		protocol.FileInfo{Name: "dir.file"},
+		protocol.FileInfo{Name: "dir/file"},
+	}
+
+	replace(s, protocol.LocalDeviceID, localHave)
+
+	for _, prefix := range []string{"dir", "dir/"} {
+		pl := haveListPrefixed(s, protocol.LocalDeviceID, prefix)
+		if l := len(pl); l != 2 {
+			t.Errorf("Expected 2, got %v local items below %v", l, prefix)
+		}
+		pl = globalListPrefixed(s, prefix)
+		if l := len(pl); l != 2 {
+			t.Errorf("Expected 2, got %v global items below %v", l, prefix)
+		}
+	}
+}
+
 func replace(fs *db.FileSet, device protocol.DeviceID, files []protocol.FileInfo) {
 	fs.Drop(device)
 	fs.Update(device, files)

lib/model/model.go

@@ -2403,7 +2403,8 @@ func (m *Model) GlobalDirectoryTree(folder, prefix string, levels int, dirsonly
 	files.WithPrefixedGlobalTruncated(prefix, func(fi db.FileIntf) bool {
 		f := fi.(db.FileInfoTruncated)
 
-		if f.IsInvalid() || f.IsDeleted() || f.Name == prefix {
+		// Don't include the prefix itself.
+		if f.IsInvalid() || f.IsDeleted() || strings.HasPrefix(prefix, f.Name) {
 			return true
 		}