From 39438736263206ab6dee58989ef09c9d220b5e32 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 205ea7e0f..c840386f4 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)