From 3ffbe45a6d767d4259cf1fb9b96d0307deb8042b Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Thu, 27 Aug 2020 18:33:27 +0200 Subject: [PATCH] lib/db, lib/model: Avoid accounting mishap on folder restart (fixes #6938) (#6939) We created a new fileset before stopping the folder during restart. When we create that fileset it loads the current metadata and sequence numbers from the database. But the folder may have time to update those before stopping, leaving the new fileset with bad data. This would cause wrong accounting (forgotten files) and potentially sequence reuse causing files not sent to other devices. This change reuses the fileset on restart, skipping the issue entirely. It also moves the creation of the fileset back under the lock so there should be no chance of concurrency issues here. --- lib/model/model.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/model/model.go b/lib/model/model.go index 9b9737c2e..a7eeb350e 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -452,6 +452,7 @@ func (m *model) restartFolder(from, to config.FolderConfiguration, cacheIgnoredF l.Warnf("bug: folder restart cannot change ID %q -> %q", from.ID, to.ID) panic("bug: folder restart cannot change ID") } + folder := to.ID // This mutex protects the entirety of the restart operation, preventing // there from being more than one folder restart operation in progress @@ -459,7 +460,7 @@ func (m *model) restartFolder(from, to config.FolderConfiguration, cacheIgnoredF // because those locks are released while we are waiting for the folder // to shut down (and must be so because the folder might need them as // part of its operations before shutting down). - restartMut := m.folderRestartMuts.Get(to.ID) + restartMut := m.folderRestartMuts.Get(folder) restartMut.Lock() defer restartMut.Unlock() @@ -477,13 +478,6 @@ func (m *model) restartFolder(from, to config.FolderConfiguration, cacheIgnoredF errMsg = "restarting" } - var fset *db.FileSet - if !to.Paused { - // Creating the fileset can take a long time (metadata calculation) - // so we do it outside of the lock. - fset = db.NewFileSet(to.ID, to.Filesystem(), m.db) - } - err := fmt.Errorf("%v folder %v", errMsg, to.Description()) m.stopFolder(from, err) // Need to send CC change to both from and to devices. @@ -492,8 +486,17 @@ func (m *model) restartFolder(from, to config.FolderConfiguration, cacheIgnoredF m.fmut.Lock() defer m.fmut.Unlock() + // Cache the (maybe) existing fset before it's removed by cleanupFolderLocked + fset := m.folderFiles[folder] + m.cleanupFolderLocked(from) if !to.Paused { + if fset == nil { + // Create a new fset. Might take a while and we do it under + // locking, but it's unsafe to create fset:s concurrently so + // that's the price we pay. + fset = db.NewFileSet(folder, to.Filesystem(), m.db) + } m.addAndStartFolderLocked(to, fset, cacheIgnoredFiles) } l.Infof("%v folder %v (%v)", infoMsg, to.Description(), to.Type)