lib/model: Fix removal of paused folders, improve tests (fixes #4405)

lib/config/wrapper.go

@@ -128,15 +128,26 @@ func (w *Wrapper) RawCopy() Configuration {
 	return w.cfg.Copy()
 }
 
+// ReplaceBlocking swaps the current configuration object for the given one,
+// and waits for subscribers to be notified.
+func (w *Wrapper) ReplaceBlocking(cfg Configuration) error {
+	w.mut.Lock()
+	wg := sync.NewWaitGroup()
+	err := w.replaceLocked(cfg, wg)
+	w.mut.Unlock()
+	wg.Wait()
+	return err
+}
+
 // Replace swaps the current configuration object for the given one.
 func (w *Wrapper) Replace(cfg Configuration) error {
 	w.mut.Lock()
 	defer w.mut.Unlock()
 
-	return w.replaceLocked(cfg)
+	return w.replaceLocked(cfg, nil)
 }
 
-func (w *Wrapper) replaceLocked(to Configuration) error {
+func (w *Wrapper) replaceLocked(to Configuration, wg sync.WaitGroup) error {
 	from := w.cfg
 
 	if err := to.clean(); err != nil {
@@ -155,14 +166,22 @@ func (w *Wrapper) replaceLocked(to Configuration) error {
 	w.deviceMap = nil
 	w.folderMap = nil
 
-	w.notifyListeners(from, to)
+	w.notifyListeners(from, to, wg)
 
 	return nil
 }
 
-func (w *Wrapper) notifyListeners(from, to Configuration) {
+func (w *Wrapper) notifyListeners(from, to Configuration, wg sync.WaitGroup) {
+	if wg != nil {
+		wg.Add(len(w.subs))
+	}
 	for _, sub := range w.subs {
-		go w.notifyListener(sub, from.Copy(), to.Copy())
+		go func(commiter Committer) {
+			w.notifyListener(commiter, from.Copy(), to.Copy())
+			if wg != nil {
+				wg.Done()
+			}
+		}(sub)
 	}
 }
 
@@ -210,7 +229,7 @@ func (w *Wrapper) SetDevices(devs []DeviceConfiguration) error {
 		}
 	}
 
-	return w.replaceLocked(newCfg)
+	return w.replaceLocked(newCfg, nil)
 }
 
 // SetDevice adds a new device to the configuration, or overwrites an existing
@@ -237,7 +256,7 @@ func (w *Wrapper) RemoveDevice(id protocol.DeviceID) error {
 		return nil
 	}
 
-	return w.replaceLocked(newCfg)
+	return w.replaceLocked(newCfg, nil)
 }
 
 // Folders returns a map of folders. Folder structures should not be changed,
@@ -273,7 +292,7 @@ func (w *Wrapper) SetFolder(fld FolderConfiguration) error {
 		newCfg.Folders = append(w.cfg.Folders, fld)
 	}
 
-	return w.replaceLocked(newCfg)
+	return w.replaceLocked(newCfg, nil)
 }
 
 // Options returns the current options configuration object.
@@ -289,7 +308,7 @@ func (w *Wrapper) SetOptions(opts OptionsConfiguration) error {
 	defer w.mut.Unlock()
 	newCfg := w.cfg.Copy()
 	newCfg.Options = opts
-	return w.replaceLocked(newCfg)
+	return w.replaceLocked(newCfg, nil)
 }
 
 // GUI returns the current GUI configuration object.
@@ -305,7 +324,7 @@ func (w *Wrapper) SetGUI(gui GUIConfiguration) error {
 	defer w.mut.Unlock()
 	newCfg := w.cfg.Copy()
 	newCfg.GUI = gui
-	return w.replaceLocked(newCfg)
+	return w.replaceLocked(newCfg, nil)
 }
 
 // IgnoredDevice returns whether or not connection attempts from the given

lib/model/model.go

@@ -335,25 +335,15 @@ func (m *Model) addFolderLocked(cfg config.FolderConfiguration) {
 	m.folderIgnores[cfg.ID] = ignores
 }
 
-func (m *Model) RemoveFolder(folder string) {
+func (m *Model) RemoveFolder(cfg config.FolderConfiguration) {
 	m.fmut.Lock()
 	m.pmut.Lock()
-
 	// Delete syncthing specific files
-	folderCfg, ok := m.folderCfgs[folder]
-	if !ok {
-		// Folder might be paused
-		folderCfg, ok = m.cfg.Folder(folder)
-	}
-	if !ok {
-		panic("bug: remove non existing folder")
-	}
-	fs := folderCfg.Filesystem()
-	fs.RemoveAll(".stfolder")
+	cfg.Filesystem().RemoveAll(".stfolder")
 
-	m.tearDownFolderLocked(folder)
+	m.tearDownFolderLocked(cfg.ID)
 	// Remove it from the database
-	db.DropFolder(m.db, folder)
+	db.DropFolder(m.db, cfg.ID)
 
 	m.pmut.Unlock()
 	m.fmut.Unlock()
@@ -2496,7 +2486,7 @@ func (m *Model) CommitConfiguration(from, to config.Configuration) bool {
 		toCfg, ok := toFolders[folderID]
 		if !ok {
 			// The folder was removed.
-			m.RemoveFolder(folderID)
+			m.RemoveFolder(fromCfg)
 			continue
 		}
 

lib/model/model_test.go

@@ -1907,48 +1907,69 @@ func TestIssue3164(t *testing.T) {
 
 func TestIssue4357(t *testing.T) {
 	db := db.OpenMemory()
-	m := NewModel(defaultConfig, protocol.LocalDeviceID, "syncthing", "dev", db, nil)
+	cfg := defaultConfig.RawCopy()
+	// Create a separate wrapper not to polute other tests.
+	wrapper := config.Wrap("/tmp/test", config.Configuration{})
+	m := NewModel(wrapper, protocol.LocalDeviceID, "syncthing", "dev", db, nil)
 	m.ServeBackground()
 	defer m.Stop()
-	cfg := m.cfg.RawCopy()
-	m.CommitConfiguration(config.Configuration{}, cfg)
+
+	// Replace kicks off CommitConfiguration in multiple routines which we
+	// have no idea when they get called, so sleep a bit :(
+	// We could make replace blocking for all routines to come back without
+	// holding the lock, but that's a bit much just to make a test.
+	replaceCfg := func(cfg config.Configuration) {
+		t.Helper()
+		if err := wrapper.ReplaceBlocking(cfg); err != nil {
+			t.Error(err)
+		}
+	}
+
+	// Force the model to wire itself and add the folders
+	replaceCfg(cfg)
 
 	if _, ok := m.folderCfgs["default"]; !ok {
 		t.Error("Folder should be running")
 	}
 
-	newCfg := m.cfg.RawCopy()
+	newCfg := wrapper.RawCopy()
 	newCfg.Folders[0].Paused = true
-	m.CommitConfiguration(cfg, newCfg)
+
+	replaceCfg(newCfg)
 
 	if _, ok := m.folderCfgs["default"]; ok {
 		t.Error("Folder should not be running")
 	}
 
-	// Should not panic when removing a paused folder.
-	m.RemoveFolder("default")
+	if _, ok := m.cfg.Folder("default"); !ok {
+		t.Error("should still have folder in config")
+	}
+
+	replaceCfg(config.Configuration{})
+
+	if _, ok := m.cfg.Folder("default"); ok {
+		t.Error("should not have folder in config")
+	}
 
 	// Add the folder back, should be running
-	m.CommitConfiguration(config.Configuration{}, cfg)
+	replaceCfg(cfg)
 
 	if _, ok := m.folderCfgs["default"]; !ok {
 		t.Error("Folder should be running")
 	}
+	if _, ok := m.cfg.Folder("default"); !ok {
+		t.Error("should still have folder in config")
+	}
 
 	// Should not panic when removing a running folder.
-	m.RemoveFolder("default")
+	replaceCfg(config.Configuration{})
+
 	if _, ok := m.folderCfgs["default"]; ok {
 		t.Error("Folder should not be running")
 	}
-
-	// Should panic when removing a non-existing folder
-	defer func() {
-		if recover() == nil {
-			t.Error("expected a panic")
-		}
-	}()
-	m.RemoveFolder("non-existing")
-
+	if _, ok := m.cfg.Folder("default"); ok {
+		t.Error("should not have folder in config")
+	}
 }
 
 func TestScanNoDatabaseWrite(t *testing.T) {