lib/model: Fixes on receive-only test setup and pulling (#5136)

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/debug.go

@@ -20,3 +20,7 @@ var (
 func init() {
 	l.SetDebug("db", strings.Contains(os.Getenv("STTRACE"), "db") || os.Getenv("STTRACE") == "all")
 }
+
+func shouldDebug() bool {
+	return l.ShouldDebug("db")
+}

lib/db/leveldb_dbinstance.go

@@ -221,6 +221,14 @@ func (db *Instance) withHaveSequence(folder []byte, startSeq int64, fn Iterator)
 			l.Debugln("missing file for sequence number", db.sequenceKeySequence(dbi.Key()))
 			continue
 		}
+
+		if shouldDebug() {
+			key := dbi.Key()
+			seq := int64(binary.BigEndian.Uint64(key[keyPrefixLen+keyFolderLen:]))
+			if f.Sequence != seq {
+				panic(fmt.Sprintf("sequence index corruption, file sequence %d != expected %d", f.Sequence, seq))
+			}
+		}
 		if !fn(f) {
 			return
 		}

lib/db/set.go

@@ -14,6 +14,7 @@ package db
 
 import (
 	"os"
+	"sort"
 	"time"
 
 	"github.com/syncthing/syncthing/lib/fs"
@@ -139,38 +140,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) {

lib/db/set_test.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"sort"
 	"testing"
+	"time"
 
 	"github.com/d4l3k/messagediff"
 	"github.com/syncthing/syncthing/lib/db"
@@ -914,6 +915,56 @@ func TestWithHaveSequence(t *testing.T) {
 	})
 }
 
+func TestStressWithHaveSequence(t *testing.T) {
+	// This races two loops against each other: one that contiously does
+	// updates, and one that continously does sequence walks. The test fails
+	// if the sequence walker sees a discontinuity.
+
+	if testing.Short() {
+		t.Skip("Takes a long time")
+	}
+
+	ldb := db.OpenMemory()
+
+	folder := "test"
+	s := db.NewFileSet(folder, fs.NewFilesystem(fs.FilesystemTypeBasic, "."), ldb)
+
+	var localHave []protocol.FileInfo
+	for i := 0; i < 100; i++ {
+		localHave = append(localHave, protocol.FileInfo{Name: fmt.Sprintf("file%d", i), Blocks: genBlocks(i * 10)})
+	}
+
+	done := make(chan struct{})
+	t0 := time.Now()
+	go func() {
+		for time.Since(t0) < 10*time.Second {
+			for j, f := range localHave {
+				localHave[j].Version = f.Version.Update(42)
+			}
+
+			s.Update(protocol.LocalDeviceID, localHave)
+		}
+		close(done)
+	}()
+
+	var prevSeq int64 = 0
+loop:
+	for {
+		select {
+		case <-done:
+			break loop
+		default:
+		}
+		s.WithHaveSequence(prevSeq+1, func(fi db.FileIntf) bool {
+			if fi.SequenceNo() < prevSeq+1 {
+				t.Fatal("Skipped ", prevSeq+1, fi.SequenceNo())
+			}
+			prevSeq = fi.SequenceNo()
+			return true
+		})
+	}
+}
+
 func TestIssue4925(t *testing.T) {
 	ldb := db.OpenMemory()
 

lib/fs/basicfs.go

@@ -25,8 +25,7 @@ var (
 // The BasicFilesystem implements all aspects by delegating to package os.
 // All paths are relative to the root and cannot (should not) escape the root directory.
 type BasicFilesystem struct {
-	root                 string
-	rootSymlinkEvaluated string
+	root string
 }
 
 func newBasicFilesystem(root string) *BasicFilesystem {
@@ -36,7 +35,8 @@ func newBasicFilesystem(root string) *BasicFilesystem {
 	// C:\somedir\ ->  C:\somedir\\   ->  C:\somedir
 	// This way in the tests, we get away without OS specific separators
 	// in the test configs.
-	root = filepath.Dir(root + string(filepath.Separator))
+	sep := string(filepath.Separator)
+	root = filepath.Dir(root + sep)
 
 	// Attempt tilde expansion; leave unchanged in case of error
 	if path, err := ExpandTilde(root); err == nil {
@@ -53,34 +53,13 @@ func newBasicFilesystem(root string) *BasicFilesystem {
 		}
 	}
 
-	rootSymlinkEvaluated, err := filepath.EvalSymlinks(root)
-	if err != nil {
-		rootSymlinkEvaluated = root
-	}
-
-	return &BasicFilesystem{
-		root:                 adjustRoot(root),
-		rootSymlinkEvaluated: adjustRoot(rootSymlinkEvaluated),
-	}
-}
-
-func adjustRoot(root string) string {
 	// Attempt to enable long filename support on Windows. We may still not
 	// have an absolute path here if the previous steps failed.
 	if runtime.GOOS == "windows" {
-		if filepath.IsAbs(root) && !strings.HasPrefix(root, `\\`) {
-			root = `\\?\` + root
-		}
-		return root
+		root = longFilenameSupport(root)
 	}
 
-	// If we're not on Windows, we want the path to end with a slash to
-	// penetrate symlinks. On Windows, paths must not end with a slash.
-	if root[len(root)-1] != filepath.Separator {
-		root = root + string(filepath.Separator)
-	}
-
-	return root
+	return &BasicFilesystem{root}
 }
 
 // rooted expands the relative path to the full path that is then used with os
@@ -91,57 +70,26 @@ func (f *BasicFilesystem) rooted(rel string) (string, error) {
 	return rooted(rel, f.root)
 }
 
-// rootedSymlinkEvaluated does the same as rooted, but the returned path will not
-// contain any symlinks.  package. If the relative path somehow causes the final
-// path to escape the root directory, this returns an error, to prevent accessing
-// files that are not in the shared directory.
-func (f *BasicFilesystem) rootedSymlinkEvaluated(rel string) (string, error) {
-	return rooted(rel, f.rootSymlinkEvaluated)
-}
-
 func rooted(rel, root string) (string, error) {
 	// The root must not be empty.
 	if root == "" {
 		return "", ErrInvalidFilename
 	}
 
-	pathSep := string(PathSeparator)
-
-	// The expected prefix for the resulting path is the root, with a path
-	// separator at the end.
-	expectedPrefix := filepath.FromSlash(root)
-	if !strings.HasSuffix(expectedPrefix, pathSep) {
-		expectedPrefix += pathSep
-	}
-
 	var err error
+	// Takes care that rel does not try to escape
 	rel, err = Canonicalize(rel)
 	if err != nil {
 		return "", err
 	}
 
-	// The supposedly correct path is the one filepath.Join will return, as
-	// it does cleaning and so on. Check that one first to make sure no
-	// obvious escape attempts have been made.
-	joined := filepath.Join(root, rel)
-	if rel == "." && !strings.HasSuffix(joined, pathSep) {
-		joined += pathSep
-	}
-	if !strings.HasPrefix(joined, expectedPrefix) {
-		return "", ErrNotRelative
-	}
-
-	return joined, nil
+	return filepath.Join(root, rel), nil
 }
 
 func (f *BasicFilesystem) unrooted(path string) string {
 	return rel(path, f.root)
 }
 
-func (f *BasicFilesystem) unrootedSymlinkEvaluated(path string) string {
-	return rel(path, f.rootSymlinkEvaluated)
-}
-
 func rel(path, prefix string) string {
 	return strings.TrimPrefix(strings.TrimPrefix(path, prefix), string(PathSeparator))
 }
@@ -372,3 +320,13 @@ func (e fsFileInfo) IsRegular() bool {
 	// Must use fsFileInfo.Mode() because it may apply magic.
 	return e.Mode()&ModeType == 0
 }
+
+// longFilenameSupport adds the necessary prefix to the path to enable long
+// filename support on windows if necessary.
+// This does NOT check the current system, i.e. will also take effect on unix paths.
+func longFilenameSupport(path string) string {
+	if filepath.IsAbs(path) && !strings.HasPrefix(path, `\\`) {
+		return `\\?\` + path
+	}
+	return path
+}

lib/fs/basicfs_test.go

@@ -12,6 +12,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 )
@@ -331,10 +332,14 @@ func TestRooted(t *testing.T) {
 		{"baz/foo/", "/bar/baz", "baz/foo/bar/baz", true},
 
 		// Not escape attempts, but oddly formatted relative paths.
-		{"foo", "", "foo/", true},
-		{"foo", "/", "foo/", true},
-		{"foo", "/..", "foo/", true},
+		{"foo", "", "foo", true},
+		{"foo", "/", "foo", true},
+		{"foo", "/..", "foo", true},
 		{"foo", "./bar", "foo/bar", true},
+		{"foo/", "", "foo", true},
+		{"foo/", "/", "foo", true},
+		{"foo/", "/..", "foo", true},
+		{"foo/", "./bar", "foo/bar", true},
 		{"baz/foo", "./bar", "baz/foo/bar", true},
 		{"foo", "./bar/baz", "foo/bar/baz", true},
 		{"baz/foo", "./bar/baz", "baz/foo/bar/baz", true},
@@ -395,6 +400,10 @@ func TestRooted(t *testing.T) {
 			{`\\?\c:\`, `\\foo`, ``, false},
 			{`\\?\c:\`, ``, `\\?\c:\`, true},
 			{`\\?\c:\`, `\`, `\\?\c:\`, true},
+			{`\\?\c:\test`, `.`, `\\?\c:\test`, true},
+			{`c:\test`, `.`, `c:\test`, true},
+			{`\\?\c:\test`, `/`, `\\?\c:\test`, true},
+			{`c:\test`, ``, `c:\test`, true},
 
 			// makes no sense, but will be treated simply as a bad filename
 			{`c:\foo`, `d:\bar`, `c:\foo\d:\bar`, true},
@@ -449,3 +458,35 @@ func TestRooted(t *testing.T) {
 		}
 	}
 }
+
+func TestNewBasicFilesystem(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("non-windows root paths")
+	}
+
+	testCases := []struct {
+		input        string
+		expectedRoot string
+		expectedURI  string
+	}{
+		{"/foo/bar/baz", "/foo/bar/baz", "/foo/bar/baz"},
+		{"/foo/bar/baz/", "/foo/bar/baz", "/foo/bar/baz"},
+		{"", "/", "/"},
+		{"/", "/", "/"},
+	}
+
+	for _, testCase := range testCases {
+		fs := newBasicFilesystem(testCase.input)
+		if fs.root != testCase.expectedRoot {
+			t.Errorf("root %q != %q", fs.root, testCase.expectedRoot)
+		}
+		if fs.URI() != testCase.expectedURI {
+			t.Errorf("uri %q != %q", fs.URI(), testCase.expectedURI)
+		}
+	}
+
+	fs := newBasicFilesystem("relative/path")
+	if fs.root == "relative/path" || !strings.HasPrefix(fs.root, string(PathSeparator)) {
+		t.Errorf(`newBasicFilesystem("relative/path").root == %q, expected absolutification`, fs.root)
+	}
+}

lib/fs/basicfs_watch.go

@@ -13,6 +13,7 @@ import (
 	"errors"
 	"fmt"
 	"path/filepath"
+	"runtime"
 	"strings"
 
 	"github.com/syncthing/notify"
@@ -24,7 +25,15 @@ import (
 var backendBuffer = 500
 
 func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context, ignorePerms bool) (<-chan Event, error) {
-	absName, err := f.rootedSymlinkEvaluated(name)
+	evalRoot, err := filepath.EvalSymlinks(f.root)
+	if err != nil {
+		return nil, err
+	}
+	if runtime.GOOS == "windows" {
+		evalRoot = longFilenameSupport(evalRoot)
+	}
+
+	absName, err := rooted(name, evalRoot)
 	if err != nil {
 		return nil, err
 	}
@@ -39,7 +48,7 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context
 
 	if ignore.SkipIgnoredDirs() {
 		absShouldIgnore := func(absPath string) bool {
-			return ignore.ShouldIgnore(f.unrootedChecked(absPath))
+			return ignore.ShouldIgnore(f.unrootedChecked(absPath, evalRoot))
 		}
 		err = notify.WatchWithFilter(filepath.Join(absName, "..."), backendChan, absShouldIgnore, eventMask)
 	} else {
@@ -53,12 +62,12 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context
 		return nil, err
 	}
 
-	go f.watchLoop(name, backendChan, outChan, ignore, ctx)
+	go f.watchLoop(name, evalRoot, backendChan, outChan, ignore, ctx)
 
 	return outChan, nil
 }
 
-func (f *BasicFilesystem) watchLoop(name string, backendChan chan notify.EventInfo, outChan chan<- Event, ignore Matcher, ctx context.Context) {
+func (f *BasicFilesystem) watchLoop(name, evalRoot string, backendChan chan notify.EventInfo, outChan chan<- Event, ignore Matcher, ctx context.Context) {
 	for {
 		// Detect channel overflow
 		if len(backendChan) == backendBuffer {
@@ -77,7 +86,7 @@ func (f *BasicFilesystem) watchLoop(name string, backendChan chan notify.EventIn
 
 		select {
 		case ev := <-backendChan:
-			relPath := f.unrootedChecked(ev.Path())
+			relPath := f.unrootedChecked(ev.Path(), evalRoot)
 			if ignore.ShouldIgnore(relPath) {
 				l.Debugln(f.Type(), f.URI(), "Watch: Ignoring", relPath)
 				continue
@@ -110,12 +119,13 @@ func (f *BasicFilesystem) eventType(notifyType notify.Event) EventType {
 // unrooted). It panics if the given path is not a subpath and handles the
 // special case when the given path is the folder root without a trailing
 // pathseparator.
-func (f *BasicFilesystem) unrootedChecked(absPath string) string {
-	if absPath+string(PathSeparator) == f.rootSymlinkEvaluated {
+func (f *BasicFilesystem) unrootedChecked(absPath, root string) string {
+	absPath = f.resolveWin83(absPath)
+	if absPath+string(PathSeparator) == root {
 		return "."
 	}
-	if !strings.HasPrefix(absPath, f.rootSymlinkEvaluated) {
-		panic(fmt.Sprintf("bug: Notify backend is processing a change outside of the filesystem root: root==%v, rootSymEval==%v, path==%v", f.root, f.rootSymlinkEvaluated, absPath))
+	if !strings.HasPrefix(absPath, root) {
+		panic(fmt.Sprintf("bug: Notify backend is processing a change outside of the filesystem root: f.root==%v, root==%v, path==%v", f.root, root, absPath))
 	}
-	return f.unrootedSymlinkEvaluated(f.resolveWin83(absPath))
+	return rel(absPath, root)
 }

lib/fs/basicfs_watch_test.go

@@ -33,11 +33,17 @@ func TestMain(m *testing.M) {
 	if err != nil {
 		panic("Cannot get absolute path to working dir")
 	}
+
 	dir, err = filepath.EvalSymlinks(dir)
 	if err != nil {
 		panic("Cannot get real path to working dir")
 	}
+
 	testDirAbs = filepath.Join(dir, testDir)
+	if runtime.GOOS == "windows" {
+		testDirAbs = longFilenameSupport(testDirAbs)
+	}
+
 	testFs = NewFilesystem(FilesystemTypeBasic, testDirAbs)
 
 	backendBuffer = 10
@@ -154,7 +160,7 @@ func TestWatchOutside(t *testing.T) {
 			}
 			cancel()
 		}()
-		fs.watchLoop(".", backendChan, outChan, fakeMatcher{}, ctx)
+		fs.watchLoop(".", testDirAbs, backendChan, outChan, fakeMatcher{}, ctx)
 	}()
 
 	backendChan <- fakeEventInfo(filepath.Join(filepath.Dir(testDirAbs), "outside"))
@@ -177,7 +183,7 @@ func TestWatchSubpath(t *testing.T) {
 	fs := newBasicFilesystem(testDirAbs)
 
 	abs, _ := fs.rooted("sub")
-	go fs.watchLoop("sub", backendChan, outChan, fakeMatcher{}, ctx)
+	go fs.watchLoop("sub", testDirAbs, backendChan, outChan, fakeMatcher{}, ctx)
 
 	backendChan <- fakeEventInfo(filepath.Join(abs, "file"))
 
@@ -291,7 +297,7 @@ func TestUnrootedChecked(t *testing.T) {
 		}
 	}()
 	fs := newBasicFilesystem(testDirAbs)
-	unrooted = fs.unrootedChecked("/random/other/path")
+	unrooted = fs.unrootedChecked("/random/other/path", testDirAbs)
 }
 
 func TestWatchIssue4877(t *testing.T) {

lib/fs/basicfs_windows.go

@@ -170,12 +170,12 @@ func (f *BasicFilesystem) resolveWin83(absPath string) string {
 	}
 	// Failed getting the long path. Return the part of the path which is
 	// already a long path.
-	for absPath = filepath.Dir(absPath); strings.HasPrefix(absPath, f.rootSymlinkEvaluated); absPath = filepath.Dir(absPath) {
+	for absPath = filepath.Dir(absPath); strings.HasPrefix(absPath, f.root); absPath = filepath.Dir(absPath) {
 		if !isMaybeWin83(absPath) {
 			return absPath
 		}
 	}
-	return f.rootSymlinkEvaluated
+	return f.root
 }
 
 func isMaybeWin83(absPath string) bool {

lib/model/folder_recvonly_test.go

@@ -7,6 +7,8 @@
 package model
 
 import (
+	"bytes"
+	"context"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -16,6 +18,7 @@ import (
 	"github.com/syncthing/syncthing/lib/db"
 	"github.com/syncthing/syncthing/lib/fs"
 	"github.com/syncthing/syncthing/lib/protocol"
+	"github.com/syncthing/syncthing/lib/scanner"
 )
 
 func TestRecvOnlyRevertDeletes(t *testing.T) {
@@ -218,6 +221,7 @@ func setupKnownFiles(t *testing.T, data []byte) []protocol.FileInfo {
 	if err != nil {
 		t.Fatal(err)
 	}
+	blocks, _ := scanner.Blocks(context.TODO(), bytes.NewReader(data), protocol.BlockSize(int64(len(data))), int64(len(data)), nil, true)
 	knownFiles := []protocol.FileInfo{
 		{
 			Name:        "knownDir",
@@ -235,6 +239,7 @@ func setupKnownFiles(t *testing.T, data []byte) []protocol.FileInfo {
 			ModifiedNs:  int32(fi.ModTime().UnixNano() % 1e9),
 			Version:     protocol.Vector{Counters: []protocol.Counter{{ID: 42, Value: 42}}},
 			Sequence:    42,
+			Blocks:      blocks,
 		},
 	}
 

lib/model/folder_sendrecv.go

@@ -458,6 +458,7 @@ nextFile:
 		}
 
 		if !f.checkParent(fi.Name, scanChan) {
+			f.queue.Done(fileName)
 			continue
 		}
 

lib/model/model.go

@@ -1765,6 +1765,15 @@ func sendIndexTo(prevSequence int64, conn protocol.Connection, folder string, fs
 			batchSizeBytes = 0
 		}
 
+		if shouldDebug() {
+			if fi.SequenceNo() < prevSequence+1 {
+				panic(fmt.Sprintln("sequence lower than requested, got:", fi.SequenceNo(), ", asked to start at:", prevSequence+1))
+			}
+			if f.Sequence > 0 && fi.SequenceNo() <= f.Sequence {
+				panic(fmt.Sprintln("non-increasing sequence, current:", fi.SequenceNo(), "<= previous:", f.Sequence))
+			}
+		}
+
 		f = fi.(protocol.FileInfo)
 
 		// Mark the file as invalid if any of the local bad stuff flags are set.