From a28de730318bf93b8ea9363f9bf2a05c178b4033 Mon Sep 17 00:00:00 2001
From: Simon Frei <freisim93@gmail.com>
Date: Fri, 8 Dec 2023 07:13:09 +0100
Subject: [PATCH] lib/model: Remove runner during folder cleanup (fixes #9269)
 (#9271)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before introducing the service map and using it for folder runners, the
entries in folderCfgs and folderRunners for the same key/folder were
removed under a single lock. Stopping the folder happens separately
before that with just the read lock. Now with the service map stopping
the folder and removing it from the map is a single operation. And that
still happens with just a read-lock. However even with a full lock it’s
still problematic: After the folder stopped, the runner isn’t present
anymore while the folder-config still is and sais the folder isn't
paused.

The index handler in turn looks at the folder config that is not paused,
thus assumes the runner has to be present -> nil deref on the runner.

A better solution might be to push most of these fmut maps into the
folder - they anyway are needed in there. Then there's just a single
map/source of info that's necessarily consistent. That's quite a bit of
work though, and probably/likely there will be corner cases there too.
---
 lib/model/model.go       |  5 +++--
 lib/model/service_map.go | 40 +++++++++++++++++++++++++++++++---------
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/lib/model/model.go b/lib/model/model.go
index 4cc94238c..878ef9d73 100644
--- a/lib/model/model.go
+++ b/lib/model/model.go
@@ -465,7 +465,7 @@ func (m *model) warnAboutOverwritingProtectedFiles(cfg config.FolderConfiguratio
 
 func (m *model) removeFolder(cfg config.FolderConfiguration) {
 	m.fmut.RLock()
-	wait := m.folderRunners.RemoveAndWaitChan(cfg.ID, 0)
+	wait := m.folderRunners.StopAndWaitChan(cfg.ID, 0)
 	m.fmut.RUnlock()
 	<-wait
 
@@ -507,6 +507,7 @@ func (m *model) removeFolder(cfg config.FolderConfiguration) {
 // Need to hold lock on m.fmut when calling this.
 func (m *model) cleanupFolderLocked(cfg config.FolderConfiguration) {
 	// clear up our config maps
+	m.folderRunners.Remove(cfg.ID)
 	delete(m.folderCfgs, cfg.ID)
 	delete(m.folderFiles, cfg.ID)
 	delete(m.folderIgnores, cfg.ID)
@@ -536,7 +537,7 @@ func (m *model) restartFolder(from, to config.FolderConfiguration, cacheIgnoredF
 	defer restartMut.Unlock()
 
 	m.fmut.RLock()
-	wait := m.folderRunners.RemoveAndWaitChan(from.ID, 0)
+	wait := m.folderRunners.StopAndWaitChan(from.ID, 0)
 	m.fmut.RUnlock()
 	<-wait
 
diff --git a/lib/model/service_map.go b/lib/model/service_map.go
index 988ad520a..0eed99cfc 100644
--- a/lib/model/service_map.go
+++ b/lib/model/service_map.go
@@ -59,6 +59,34 @@ func (s *serviceMap[K, S]) Get(k K) (v S, ok bool) {
 	return
 }
 
+// Stop removes the service at the given key from the supervisor, stopping it.
+// The service itself is still retained, i.e. a call to Get with the same key
+// will still return a result.
+func (s *serviceMap[K, S]) Stop(k K) {
+	if tok, ok := s.tokens[k]; ok {
+		s.supervisor.Remove(tok)
+	}
+	return
+}
+
+// StopAndWaitChan removes the service at the given key from the supervisor,
+// stopping it. The service itself is still retained, i.e. a call to Get with
+// the same key will still return a result.
+// The returned channel will produce precisely one error value: either the
+// return value from RemoveAndWait (possibly nil), or errSvcNotFound if the
+// service was not found.
+func (s *serviceMap[K, S]) StopAndWaitChan(k K, timeout time.Duration) <-chan error {
+	ret := make(chan error, 1)
+	if tok, ok := s.tokens[k]; ok {
+		go func() {
+			ret <- s.supervisor.RemoveAndWait(tok, timeout)
+		}()
+	} else {
+		ret <- errSvcNotFound
+	}
+	return ret
+}
+
 // Remove removes the service at the given key, stopping it on the supervisor.
 // If there is no service at the given key, nothing happens. The return value
 // indicates whether a service was removed.
@@ -66,6 +94,8 @@ func (s *serviceMap[K, S]) Remove(k K) (found bool) {
 	if tok, ok := s.tokens[k]; ok {
 		found = true
 		s.supervisor.Remove(tok)
+	} else {
+		_, found = s.services[k]
 	}
 	delete(s.services, k)
 	delete(s.tokens, k)
@@ -84,16 +114,8 @@ func (s *serviceMap[K, S]) RemoveAndWait(k K, timeout time.Duration) error {
 // value: either the return value from RemoveAndWait (possibly nil), or
 // errSvcNotFound if the service was not found.
 func (s *serviceMap[K, S]) RemoveAndWaitChan(k K, timeout time.Duration) <-chan error {
-	ret := make(chan error, 1)
-	if tok, ok := s.tokens[k]; ok {
-		go func() {
-			ret <- s.supervisor.RemoveAndWait(tok, timeout)
-		}()
-	} else {
-		ret <- errSvcNotFound
-	}
+	ret := s.StopAndWaitChan(k, timeout)
 	delete(s.services, k)
-	delete(s.tokens, k)
 	return ret
 }