lib/model: Refactor CheckFolderHealth into separate methods

While attempting to fix #2782 I thought the problem was the
CheckFolderHealth method, so I cleaned it up. That turned out not to be
the case, but I think this is better anyhow.

It also moves the "create folder and marker if the folder was empty in
the index" code to StartFolder where I think it makes better sense.

This is covered by a number of existing tests.

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3343

cmd/syncthing/main_test.go

@@ -7,151 +7,12 @@
 package main
 
 import (
-	"os"
 	"testing"
 
 	"github.com/syncthing/syncthing/lib/config"
-	"github.com/syncthing/syncthing/lib/db"
-	"github.com/syncthing/syncthing/lib/model"
 	"github.com/syncthing/syncthing/lib/protocol"
 )
 
-func TestFolderErrors(t *testing.T) {
-	// This test intentionally avoids starting the folders. If they are
-	// started, they will perform an initial scan, which will create missing
-	// folder markers and race with the stuff we do in the test.
-
-	fcfg := config.FolderConfiguration{
-		ID:      "folder",
-		RawPath: "testdata/testfolder",
-	}
-	cfg := config.Wrap("/tmp/test", config.Configuration{
-		Folders: []config.FolderConfiguration{fcfg},
-	})
-
-	for _, file := range []string{".stfolder", "testfolder/.stfolder", "testfolder"} {
-		if err := os.Remove("testdata/" + file); err != nil && !os.IsNotExist(err) {
-			t.Fatal(err)
-		}
-	}
-
-	ldb := db.OpenMemory()
-
-	// Case 1 - new folder, directory and marker created
-
-	m := model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil)
-	m.AddFolder(fcfg)
-
-	if err := m.CheckFolderHealth("folder"); err != nil {
-		t.Error("Unexpected error", cfg.Folders()["folder"].Invalid)
-	}
-
-	s, err := os.Stat("testdata/testfolder")
-	if err != nil || !s.IsDir() {
-		t.Error(err)
-	}
-
-	_, err = os.Stat("testdata/testfolder/.stfolder")
-	if err != nil {
-		t.Error(err)
-	}
-
-	if err := os.Remove("testdata/testfolder/.stfolder"); err != nil {
-		t.Fatal(err)
-	}
-	if err := os.Remove("testdata/testfolder/"); err != nil {
-		t.Fatal(err)
-	}
-
-	// Case 2 - new folder, marker created
-
-	fcfg.RawPath = "testdata/"
-	cfg = config.Wrap("/tmp/test", config.Configuration{
-		Folders: []config.FolderConfiguration{fcfg},
-	})
-
-	m = model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil)
-	m.AddFolder(fcfg)
-
-	if err := m.CheckFolderHealth("folder"); err != nil {
-		t.Error("Unexpected error", cfg.Folders()["folder"].Invalid)
-	}
-
-	_, err = os.Stat("testdata/.stfolder")
-	if err != nil {
-		t.Error(err)
-	}
-
-	if err := os.Remove("testdata/.stfolder"); err != nil {
-		t.Fatal(err)
-	}
-
-	// Case 3 - Folder marker missing
-
-	set := db.NewFileSet("folder", ldb)
-	set.Update(protocol.LocalDeviceID, []protocol.FileInfo{
-		{Name: "dummyfile"},
-	})
-
-	m = model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil)
-	m.AddFolder(fcfg)
-
-	if err := m.CheckFolderHealth("folder"); err == nil || err.Error() != "folder marker missing" {
-		t.Error("Incorrect error: Folder marker missing !=", m.CheckFolderHealth("folder"))
-	}
-
-	// Case 3.1 - recover after folder marker missing
-
-	if err = fcfg.CreateMarker(); err != nil {
-		t.Error(err)
-	}
-
-	if err := m.CheckFolderHealth("folder"); err != nil {
-		t.Error("Unexpected error", cfg.Folders()["folder"].Invalid)
-	}
-
-	// Case 4 - Folder path missing
-
-	if err := os.Remove("testdata/testfolder/.stfolder"); err != nil && !os.IsNotExist(err) {
-		t.Fatal(err)
-	}
-	if err := os.Remove("testdata/testfolder"); err != nil && !os.IsNotExist(err) {
-		t.Fatal(err)
-	}
-
-	fcfg.RawPath = "testdata/testfolder"
-	cfg = config.Wrap("testdata/subfolder", config.Configuration{
-		Folders: []config.FolderConfiguration{fcfg},
-	})
-
-	m = model.NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", ldb, nil)
-	m.AddFolder(fcfg)
-
-	if err := m.CheckFolderHealth("folder"); err == nil || err.Error() != "folder path missing" {
-		t.Error("Incorrect error: Folder path missing !=", m.CheckFolderHealth("folder"))
-	}
-
-	// Case 4.1 - recover after folder path missing
-
-	if err := os.Mkdir("testdata/testfolder", 0700); err != nil {
-		t.Fatal(err)
-	}
-
-	if err := m.CheckFolderHealth("folder"); err == nil || err.Error() != "folder marker missing" {
-		t.Error("Incorrect error: Folder marker missing !=", m.CheckFolderHealth("folder"))
-	}
-
-	// Case 4.2 - recover after missing marker
-
-	if err = fcfg.CreateMarker(); err != nil {
-		t.Error(err)
-	}
-
-	if err := m.CheckFolderHealth("folder"); err != nil {
-		t.Error("Unexpected error", cfg.Folders()["folder"].Invalid)
-	}
-}
-
 func TestShortIDCheck(t *testing.T) {
 	cfg := config.Wrap("/tmp/test", config.Configuration{
 		Devices: []config.DeviceConfiguration{

lib/model/model.go

@@ -108,6 +108,14 @@ var (
 	folderFactories = make(map[config.FolderType]folderFactory, 0)
 )
 
+// errors returned by the CheckFolderHealth method
+var (
+	errFolderPathMissing   = errors.New("folder path missing")
+	errFolderMarkerMissing = errors.New("folder marker missing")
+	errHomeDiskNoSpace     = errors.New("home disk has insufficient free space")
+	errFolderNoSpace       = errors.New("folder has insufficient free space")
+)
+
 // NewModel creates and starts a new model. The model starts in read-only mode,
 // where it sends index information to connected peers and responds to requests
 // for file data without altering the local folder in any way.
@@ -162,7 +170,7 @@ func (m *Model) StartDeadlockDetector(timeout time.Duration) {
 	deadlockDetect(m.pmut, timeout)
 }
 
-// StartFolder constrcuts the folder service and starts it.
+// StartFolder constructs the folder service and starts it.
 func (m *Model) StartFolder(folder string) {
 	m.fmut.Lock()
 	cfg, ok := m.folderCfgs[folder]
@@ -180,6 +188,26 @@ func (m *Model) StartFolder(folder string) {
 		panic(fmt.Sprintf("unknown folder type 0x%x", cfg.Type))
 	}
 
+	fs := m.folderFiles[folder]
+	v, ok := fs.LocalVersion(protocol.LocalDeviceID), true
+	indexHasFiles := ok && v > 0
+	if !indexHasFiles {
+		// It's a blank folder, so this may the first time we're looking at
+		// it. Attempt to create and tag with our marker as appropriate. We
+		// don't really do anything with errors at this point except warn -
+		// if these things don't work, we still want to start the folder and
+		// it'll show up as errored later.
+
+		if _, err := os.Stat(cfg.Path()); os.IsNotExist(err) {
+			if err := osutil.MkdirAll(cfg.Path(), 0700); err != nil {
+				l.Warnln("Creating folder:", err)
+			}
+		}
+		if err := cfg.CreateMarker(); err != nil {
+			l.Warnln("Creating folder marker:", err)
+		}
+	}
+
 	var ver versioner.Versioner
 	if len(cfg.Versioning.Type) > 0 {
 		versionerFactory, ok := versioner.Factories[cfg.Versioning.Type]
@@ -1904,53 +1932,73 @@ func (m *Model) CheckFolderHealth(id string) error {
 		return errors.New("folder does not exist")
 	}
 
-	if minFree := m.cfg.Options().MinHomeDiskFreePct; minFree > 0 {
-		if free, err := osutil.DiskFreePercentage(m.cfg.ConfigPath()); err == nil && free < minFree {
-			return errors.New("home disk has insufficient free space")
-		}
+	// Check for folder errors, with the most serious and specific first and
+	// generic ones like out of space on the home disk later. Note the
+	// inverted error flow (err==nil checks) here.
+
+	err := m.checkFolderPath(folder)
+	if err == nil {
+		err = m.checkFolderFreeSpace(folder)
+	}
+	if err == nil {
+		err = m.checkHomeDiskFree()
 	}
 
-	fi, err := os.Stat(folder.Path())
+	// Set or clear the error on the runner, which also does logging and
+	// generates events and stuff.
+	m.runnerExchangeError(folder, err)
 
-	v, ok := m.CurrentLocalVersion(id)
-	indexHasFiles := ok && v > 0
+	return err
+}
 
-	if indexHasFiles {
-		// There are files in the folder according to the index, so it must
-		// have existed and had a correct marker at some point. Verify that
-		// this is still the case.
-
-		switch {
-		case err != nil || !fi.IsDir():
-			err = errors.New("folder path missing")
-
-		case !folder.HasMarker():
-			err = errors.New("folder marker missing")
-
-		case folder.Type != config.FolderTypeReadOnly:
-			// Check for free space, if it isn't a master folder. We aren't
-			// going to change the contents of master folders, so we don't
-			// care about the amount of free space there.
-			diskFreeP, errDfp := osutil.DiskFreePercentage(folder.Path())
-			if errDfp == nil && diskFreeP < folder.MinDiskFreePct {
-				diskFreeBytes, _ := osutil.DiskFreeBytes(folder.Path())
-				str := fmt.Sprintf("insufficient free space (%d MiB, %.2f%%)", diskFreeBytes/1024/1024, diskFreeP)
-				err = errors.New(str)
-			}
-		}
-	} else {
-		// It's a blank folder, so this may the first time we're looking at
-		// it. Attempt to create and tag with our marker as appropriate.
+// checkFolderPath returns nil if the folder path exists and has the marker file.
+func (m *Model) checkFolderPath(folder config.FolderConfiguration) error {
+	if fi, err := os.Stat(folder.Path()); err != nil || !fi.IsDir() {
+		return errFolderPathMissing
+	}
 
-		if os.IsNotExist(err) {
-			err = osutil.MkdirAll(folder.Path(), 0700)
-		}
+	if !folder.HasMarker() {
+		return errFolderMarkerMissing
+	}
 
-		if err == nil && !folder.HasMarker() {
-			err = folder.CreateMarker()
-		}
+	return nil
+}
+
+// checkFolderFreeSpace returns nil if the folder has the required amount of
+// free space, or if folder free space checking is disabled.
+func (m *Model) checkFolderFreeSpace(folder config.FolderConfiguration) error {
+	if folder.MinDiskFreePct <= 0 {
+		return nil
+	}
+
+	free, err := osutil.DiskFreePercentage(folder.Path())
+	if err == nil && free < folder.MinDiskFreePct {
+		return errFolderNoSpace
+	}
+
+	return nil
+}
+
+// checkHomeDiskFree returns nil if the home disk has the required amount of
+// free space, or if home disk free space checking is disabled.
+func (m *Model) checkHomeDiskFree() error {
+	minFree := m.cfg.Options().MinHomeDiskFreePct
+	if minFree <= 0 {
+		return nil
+	}
+
+	free, err := osutil.DiskFreePercentage(m.cfg.ConfigPath())
+	if err == nil && free < minFree {
+		return errHomeDiskNoSpace
 	}
 
+	return nil
+}
+
+// runnerExchangeError sets the given error (which way be nil) on the folder
+// runner. If the error differs from any previous error, logging and events
+// happen.
+func (m *Model) runnerExchangeError(folder config.FolderConfiguration, err error) {
 	m.fmut.RLock()
 	runner, runnerExists := m.folderRunners[folder.ID]
 	m.fmut.RUnlock()
@@ -1975,8 +2023,6 @@ func (m *Model) CheckFolderHealth(id string) error {
 			runner.clearError()
 		}
 	}
-
-	return err
 }
 
 func (m *Model) ResetFolder(folder string) {