lib/db: Fix inconsistency in sequence index (fixes #5149) (#5158)

The problem here is that we would update the sequence index before
updating the FileInfos, which would result in a high sequence number
pointing to a low-sequence FileInfo. The index sender would pick up the
high sequence number, send the old file, and think everything was good.
On the receiving side the old file is a no-op and ignored. The file
remains out of sync until another update for it happens.

This fixes that by correcting the order of operations in the database
update: first we remove old sequence index entries, then we update the
FileInfos (which now don't have anything pointing to them) and then we
add the sequence indexes (which the index sender can see).

The other option is to add "proper" transactions where required at the
database layer. I actually have a branch for that, but it's literally
thousands of lines of diff and I'm putting that off for another day as
this solves the problem...

lib/db/set.go

@@ -14,6 +14,7 @@ package db
 
 import (
 	"os"
+	"sort"
 	"time"
 
 	"github.com/syncthing/syncthing/lib/fs"
@@ -140,38 +141,56 @@ func (s *FileSet) Update(device protocol.DeviceID, fs []protocol.FileInfo) {
 	s.updateMutex.Lock()
 	defer s.updateMutex.Unlock()
 
-	if device == protocol.LocalDeviceID {
-		discards := make([]protocol.FileInfo, 0, len(fs))
-		updates := make([]protocol.FileInfo, 0, len(fs))
-		// db.UpdateFiles will sort unchanged files out -> save one db lookup
-		// filter slice according to https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating
-		oldFs := fs
-		fs = fs[:0]
-		var dk []byte
-		folder := []byte(s.folder)
-		for _, nf := range oldFs {
-			dk = s.db.deviceKeyInto(dk, folder, device[:], []byte(osutil.NormalizedFilename(nf.Name)))
-			ef, ok := s.db.getFile(dk)
-			if ok && ef.Version.Equal(nf.Version) && ef.IsInvalid() == nf.IsInvalid() {
-				continue
-			}
+	defer s.meta.toDB(s.db, []byte(s.folder))
 
-			nf.Sequence = s.meta.nextSeq(protocol.LocalDeviceID)
-			fs = append(fs, nf)
-
-			if ok {
-				discards = append(discards, ef)
-			}
-			updates = append(updates, nf)
-		}
-		s.blockmap.Discard(discards)
-		s.blockmap.Update(updates)
-		s.db.removeSequences(folder, discards)
-		s.db.addSequences(folder, updates)
+	if device != protocol.LocalDeviceID {
+		// Easy case, just update the files and we're done.
+		s.db.updateFiles([]byte(s.folder), device[:], fs, s.meta)
+		return
 	}
 
+	// For the local device we have a bunch of metadata to track however...
+
+	discards := make([]protocol.FileInfo, 0, len(fs))
+	updates := make([]protocol.FileInfo, 0, len(fs))
+	// db.UpdateFiles will sort unchanged files out -> save one db lookup
+	// filter slice according to https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating
+	oldFs := fs
+	fs = fs[:0]
+	var dk []byte
+	folder := []byte(s.folder)
+	for _, nf := range oldFs {
+		dk = s.db.deviceKeyInto(dk, folder, device[:], []byte(osutil.NormalizedFilename(nf.Name)))
+		ef, ok := s.db.getFile(dk)
+		if ok && ef.Version.Equal(nf.Version) && ef.IsInvalid() == nf.IsInvalid() {
+			continue
+		}
+
+		nf.Sequence = s.meta.nextSeq(protocol.LocalDeviceID)
+		fs = append(fs, nf)
+
+		if ok {
+			discards = append(discards, ef)
+		}
+		updates = append(updates, nf)
+	}
+
+	// The ordering here is important. We first remove stuff that point to
+	// files we are going to update, then update them, then add new index
+	// pointers etc. In addition, we do the discards in reverse order so
+	// that a reader traversing the sequence index will get a consistent
+	// view up until the point they meet the writer.
+
+	sort.Slice(discards, func(a, b int) bool {
+		// n.b. "b < a" instead of the usual "a < b"
+		return discards[b].Sequence < discards[a].Sequence
+	})
+
+	s.blockmap.Discard(discards)
+	s.db.removeSequences(folder, discards)
 	s.db.updateFiles([]byte(s.folder), device[:], fs, s.meta)
-	s.meta.toDB(s.db, []byte(s.folder))
+	s.db.addSequences(folder, updates)
+	s.blockmap.Update(updates)
 }
 
 func (s *FileSet) WithNeed(device protocol.DeviceID, fn Iterator) {