octoleo/syncthing
1
0
Fork 0
mirror of https://github.com/octoleo/syncthing.git synced 2024-11-09 23:00:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

lib/db, lib/model: Avoid accounting mishap on folder restart (fixes #6938) (#6939)

Browse Source 
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.
This commit is contained in:
Jakob Borg 2020-08-27 18:33:27 +02:00
parent 15ce96f704
commit 3943873626
1 changed files with 11 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
lib/model/model.go
View File

@ -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) l.Warnf("bug: folder restart cannot change ID %q -> %q", from.ID, to.ID)
panic("bug: folder restart cannot change ID") panic("bug: folder restart cannot change ID")
} }
folder := to.ID
// This mutex protects the entirety of the restart operation, preventing // This mutex protects the entirety of the restart operation, preventing
// there from being more than one folder restart operation in progress // 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 // 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 // to shut down (and must be so because the folder might need them as
// part of its operations before shutting down). // part of its operations before shutting down).
restartMut := m.folderRestartMuts.Get(to.ID) restartMut := m.folderRestartMuts.Get(folder)
restartMut.Lock() restartMut.Lock()
defer restartMut.Unlock() defer restartMut.Unlock()
@ -477,13 +478,6 @@ func (m *model) restartFolder(from, to config.FolderConfiguration, cacheIgnoredF
errMsg = "restarting" 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()) err := fmt.Errorf("%v folder %v", errMsg, to.Description())
m.stopFolder(from, err) m.stopFolder(from, err)
// Need to send CC change to both from and to devices. // 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() m.fmut.Lock()
defer m.fmut.Unlock() defer m.fmut.Unlock()
// Cache the (maybe) existing fset before it's removed by cleanupFolderLocked
fset := m.folderFiles[folder]
m.cleanupFolderLocked(from) m.cleanupFolderLocked(from)
if !to.Paused { 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) m.addAndStartFolderLocked(to, fset, cacheIgnoredFiles)
} }
l.Infof("%v folder %v (%v)", infoMsg, to.Description(), to.Type) l.Infof("%v folder %v (%v)", infoMsg, to.Description(), to.Type)