Package: syncthing
Version: 1.12.1~ds1-2
Severity: important

As discussed on the go-team mailing list [1], I am filing this bug to
address an important fix in upstream since the version to be included in
stable.

There have been reports of full index retransfers on almost every
restart [2], which uses significant resources (network, disk io, ...).
The underlying bug may also cause inconsistencies in the displayed sync
completion, though that has not been observed.

I propose to apply the PR fixing this [3] with the patch attached.
Please indicate if you'd prefer an MR on salsa, then I'll do that
instead in the future.

[1]: https://lists.debian.org/debian-go/2021/02/msg00067.html
[2]:
https://forum.syncthing.net/t/mismatching-index-id-for-us-on-almost-every-restart-again/16036
[3]: https://github.com/syncthing/syncthing/pull/7211
>From 5ee8b8a6aa4b5366a721f0531ef4df45780a3801 Mon Sep 17 00:00:00 2001
From: Simon Frei <freisi...@gmail.com>
Date: Mon, 21 Dec 2020 11:32:59 +0100
Subject: [PATCH 1/4] lib/db: Prevent IndexID creation race (#7211)

---
 lib/db/lowlevel.go |  2 ++
 lib/db/set.go      | 33 ++++++++++++++++++++++-----------
 lib/db/set_test.go | 26 ++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/lib/db/lowlevel.go b/lib/db/lowlevel.go
index 8074564aa..347237ea4 100644
--- a/lib/db/lowlevel.go
+++ b/lib/db/lowlevel.go
@@ -65,6 +65,7 @@ type Lowlevel struct {
 	gcKeyCount         int
 	indirectGCInterval time.Duration
 	recheckInterval    time.Duration
+	oneFileSetCreated  chan struct{}
 }
 
 func NewLowlevel(backend backend.Backend, opts ...Option) *Lowlevel {
@@ -81,6 +82,7 @@ func NewLowlevel(backend backend.Backend, opts ...Option) *Lowlevel {
 		gcMut:              sync.NewRWMutex(),
 		indirectGCInterval: indirectGCDefaultInterval,
 		recheckInterval:    recheckDefaultInterval,
+		oneFileSetCreated:  make(chan struct{}),
 	}
 	for _, opt := range opts {
 		opt(db)
diff --git a/lib/db/set.go b/lib/db/set.go
index 41e1c7ecd..66bf2ccc6 100644
--- a/lib/db/set.go
+++ b/lib/db/set.go
@@ -39,13 +39,27 @@ type FileSet struct {
 type Iterator func(f protocol.FileIntf) bool
 
 func NewFileSet(folder string, fs fs.Filesystem, db *Lowlevel) *FileSet {
-	return &FileSet{
+	select {
+	case <-db.oneFileSetCreated:
+	default:
+		close(db.oneFileSetCreated)
+	}
+	s := &FileSet{
 		folder:      folder,
 		fs:          fs,
 		db:          db,
 		meta:        db.loadMetadataTracker(folder),
 		updateMutex: sync.NewMutex(),
 	}
+	if id := s.IndexID(protocol.LocalDeviceID); id == 0 {
+		// No index ID set yet. We create one now.
+		id = protocol.NewIndexID()
+		err := s.db.setIndexID(protocol.LocalDeviceID[:], []byte(s.folder), id)
+		if err != nil && !backend.IsClosed(err) {
+			fatalError(err, fmt.Sprintf("%s Creating new IndexID", s.folder), s.db)
+		}
+	}
+	return s
 }
 
 func (s *FileSet) Drop(device protocol.DeviceID) {
@@ -357,16 +371,6 @@ func (s *FileSet) IndexID(device protocol.DeviceID) protocol.IndexID {
 	} else if err != nil {
 		fatalError(err, opStr, s.db)
 	}
-	if id == 0 && device == protocol.LocalDeviceID {
-		// No index ID set yet. We create one now.
-		id = protocol.NewIndexID()
-		err := s.db.setIndexID(device[:], []byte(s.folder), id)
-		if backend.IsClosed(err) {
-			return 0
-		} else if err != nil {
-			fatalError(err, opStr, s.db)
-		}
-	}
 	return id
 }
 
@@ -432,7 +436,14 @@ func DropFolder(db *Lowlevel, folder string) {
 
 // DropDeltaIndexIDs removes all delta index IDs from the database.
 // This will cause a full index transmission on the next connection.
+// Must be called before using FileSets, i.e. before NewFileSet is called for
+// the first time.
 func DropDeltaIndexIDs(db *Lowlevel) {
+	select {
+	case <-db.oneFileSetCreated:
+		panic("DropDeltaIndexIDs must not be called after NewFileSet for the same Lowlevel")
+	default:
+	}
 	opStr := "DropDeltaIndexIDs"
 	l.Debugf(opStr)
 	dbi, err := db.NewPrefixIterator([]byte{KeyTypeIndexID})
diff --git a/lib/db/set_test.go b/lib/db/set_test.go
index e2833ffbb..89adee96d 100644
--- a/lib/db/set_test.go
+++ b/lib/db/set_test.go
@@ -1757,6 +1757,32 @@ func TestNoIndexIDResetOnDrop(t *testing.T) {
 	}
 }
 
+func TestConcurrentIndexID(t *testing.T) {
+	done := make(chan struct{})
+	var ids [2]protocol.IndexID
+	setID := func(s *db.FileSet, i int) {
+		ids[i] = s.IndexID(protocol.LocalDeviceID)
+		done <- struct{}{}
+	}
+
+	max := 100
+	if testing.Short() {
+		max = 10
+	}
+	for i := 0; i < max; i++ {
+		ldb := db.NewLowlevel(backend.OpenMemory())
+		s := db.NewFileSet("test", fs.NewFilesystem(fs.FilesystemTypeFake, ""), ldb)
+		go setID(s, 0)
+		go setID(s, 1)
+		<-done
+		<-done
+		ldb.Close()
+		if ids[0] != ids[1] {
+			t.Fatalf("IDs differ after %v rounds", i)
+		}
+	}
+}
+
 func replace(fs *db.FileSet, device protocol.DeviceID, files []protocol.FileInfo) {
 	fs.Drop(device)
 	fs.Update(device, files)
-- 
2.30.0

_______________________________________________
Pkg-go-maintainers mailing list
Pkg-go-maintainers@alioth-lists.debian.net
https://alioth-lists.debian.net/cgi-bin/mailman/listinfo/pkg-go-maintainers

Reply via email to