From 71faae67f239a946668a70c7ff4c8ceae26c7adf Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Thu, 19 Mar 2020 14:30:20 +0100 Subject: [PATCH 1/3] lib/db: Remove emptied global list in checkGlobals (fixes #6425) (#6426) --- lib/db/db_test.go | 37 +++++++++++++++++++++++++++++++++++++ lib/db/lowlevel.go | 6 +++++- lib/db/structs.go | 3 +-- lib/db/transactions.go | 4 ---- 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/lib/db/db_test.go b/lib/db/db_test.go index a80656e35..9266e99ca 100644 --- a/lib/db/db_test.go +++ b/lib/db/db_test.go @@ -440,3 +440,40 @@ func TestDowngrade(t *testing.T) { t.Fatalf("Error has %v as min Syncthing version, expected %v", err.minSyncthingVersion, dbMinSyncthingVersion) } } + +func TestCheckGlobals(t *testing.T) { + db := NewLowlevel(backend.OpenMemory()) + defer db.Close() + + fs := NewFileSet("test", fs.NewFilesystem(fs.FilesystemTypeFake, ""), db) + + // Add any file + name := "foo" + fs.Update(protocol.LocalDeviceID, []protocol.FileInfo{ + { + Name: name, + Type: protocol.FileInfoTypeFile, + Version: protocol.Vector{Counters: []protocol.Counter{{ID: 1, Value: 1001}}}, + }, + }) + + // Remove just the file entry + if err := db.dropPrefix([]byte{KeyTypeDevice}); err != nil { + t.Fatal(err) + } + + // Clean up global entry of the now missing file + if err := db.checkGlobals([]byte(fs.folder), fs.meta); err != nil { + t.Fatal(err) + } + + // Check that the global entry is gone + gk, err := db.keyer.GenerateGlobalVersionKey(nil, []byte(fs.folder), []byte(name)) + if err != nil { + t.Fatal(err) + } + _, err = db.Get(gk) + if !backend.IsNotFound(err) { + t.Error("Expected key missing error, got", err) + } +} diff --git a/lib/db/lowlevel.go b/lib/db/lowlevel.go index c5eae55a9..1ef27096d 100644 --- a/lib/db/lowlevel.go +++ b/lib/db/lowlevel.go @@ -417,7 +417,11 @@ func (db *Lowlevel) checkGlobals(folder []byte, meta *metadataTracker) error { } } - if len(newVL.Versions) != len(vl.Versions) { + if newLen := len(newVL.Versions); newLen == 0 { + if err := t.Delete(dbi.Key()); err != nil { + return err + } + } else if newLen != len(vl.Versions) { if err := t.Put(dbi.Key(), mustMarshal(&newVL)); err != nil { return err } diff --git a/lib/db/structs.go b/lib/db/structs.go index 009f0a278..af8a629af 100644 --- a/lib/db/structs.go +++ b/lib/db/structs.go @@ -257,14 +257,13 @@ func (vl VersionList) insertAt(i int, v FileVersion) VersionList { // as the removed FileVersion and the position, where that FileVersion was. // If there is no FileVersion for the given device, the position is -1. func (vl VersionList) pop(device []byte) (VersionList, FileVersion, int) { - removedAt := -1 for i, v := range vl.Versions { if bytes.Equal(v.Device, device) { vl.Versions = append(vl.Versions[:i], vl.Versions[i+1:]...) return vl, v, i } } - return vl, FileVersion{}, removedAt + return vl, FileVersion{}, -1 } func (vl VersionList) Get(device []byte) (FileVersion, bool) { diff --git a/lib/db/transactions.go b/lib/db/transactions.go index cac81f5cf..3d205a60e 100644 --- a/lib/db/transactions.go +++ b/lib/db/transactions.go @@ -485,10 +485,6 @@ func (t readWriteTransaction) updateGlobal(gk, keyBuf, folder, device []byte, fi if err != nil { return nil, false, err } - if insertedAt == -1 { - l.Debugln("update global; same version, global unchanged") - return keyBuf, false, nil - } name := []byte(file.Name) From 0060840249d6d5e6026338b3463942dcf07ce451 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Thu, 19 Mar 2020 14:32:22 +0100 Subject: [PATCH 2/3] lib/db: Fix removeFromGlobal and no filenames in error (fixes #6427) (#6428) --- lib/db/transactions.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/db/transactions.go b/lib/db/transactions.go index 3d205a60e..c417fe84d 100644 --- a/lib/db/transactions.go +++ b/lib/db/transactions.go @@ -8,19 +8,13 @@ package db import ( "bytes" - "fmt" + "errors" "github.com/syncthing/syncthing/lib/db/backend" "github.com/syncthing/syncthing/lib/protocol" ) -type errDeviceEntryMissing struct { - name string -} - -func (err errDeviceEntryMissing) Error() string { - return fmt.Sprintf("device present in global list but missing as device/fileinfo entry: %s", err.name) -} +var errEntryFromGlobalMissing = errors.New("device present in global list but missing as device/fileinfo entry") // A readOnlyTransaction represents a database snapshot. type readOnlyTransaction struct { @@ -364,7 +358,7 @@ func (t *readOnlyTransaction) withNeed(folder, device []byte, truncate bool, fn return err } if !ok { - return errDeviceEntryMissing{string(name)} + return errEntryFromGlobalMissing } if !need(gf, have, haveFV.Version) { continue @@ -632,7 +626,7 @@ func (t readWriteTransaction) removeFromGlobal(gk, keyBuf, folder, device []byte return nil, err } if f, ok, err := t.getFileByKey(keyBuf); err != nil { - return keyBuf, nil + return nil, err } else if ok { meta.removeFile(protocol.GlobalDeviceID, f) } @@ -658,8 +652,11 @@ func (t readWriteTransaction) removeFromGlobal(gk, keyBuf, folder, device []byte return nil, err } global, ok, err := t.getFileByKey(keyBuf) - if err != nil || !ok { - return keyBuf, err + if err != nil { + return nil, err + } + if !ok { + return nil, errEntryFromGlobalMissing } keyBuf, err = t.updateLocalNeed(keyBuf, folder, file, fl, global) if err != nil { From b33d5e57c6751deba48a84822f9de8127d133b01 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Thu, 19 Mar 2020 15:58:32 +0100 Subject: [PATCH 3/3] lib/db, lib/syncthing: Repair db once on upgrade (ref #6425, #6427) (#6429) --- lib/db/db_test.go | 2 +- lib/db/lowlevel.go | 111 +++++++++++++++++++++++++++++++++++++ lib/db/set.go | 103 +--------------------------------- lib/syncthing/syncthing.go | 11 +++- 4 files changed, 123 insertions(+), 104 deletions(-) diff --git a/lib/db/db_test.go b/lib/db/db_test.go index 9266e99ca..8b74a63e6 100644 --- a/lib/db/db_test.go +++ b/lib/db/db_test.go @@ -352,7 +352,7 @@ func TestRepairSequence(t *testing.T) { // Loading the metadata for the first time means a "re"calculation happens, // along which the sequences get repaired too. db.gcMut.RLock() - _ = loadMetadataTracker(db, folderStr) + _ = db.loadMetadataTracker(folderStr) db.gcMut.RUnlock() if err != nil { t.Fatal(err) diff --git a/lib/db/lowlevel.go b/lib/db/lowlevel.go index 1ef27096d..f349a64b0 100644 --- a/lib/db/lowlevel.go +++ b/lib/db/lowlevel.go @@ -627,6 +627,117 @@ func (db *Lowlevel) gcIndirect() error { return db.Compact() } +// CheckRepair checks folder metadata and sequences for miscellaneous errors. +func (db *Lowlevel) CheckRepair() { + for _, folder := range db.ListFolders() { + _ = db.getMetaAndCheck(folder) + } +} + +func (db *Lowlevel) getMetaAndCheck(folder string) *metadataTracker { + db.gcMut.RLock() + defer db.gcMut.RUnlock() + + meta, err := db.recalcMeta(folder) + if err == nil { + var fixed int + fixed, err = db.repairSequenceGCLocked(folder, meta) + if fixed != 0 { + l.Infoln("Repaired %v sequence entries in database", fixed) + } + } + + if backend.IsClosed(err) { + return nil + } else if err != nil { + panic(err) + } + + return meta +} + +func (db *Lowlevel) loadMetadataTracker(folder string) *metadataTracker { + meta := newMetadataTracker() + if err := meta.fromDB(db, []byte(folder)); err != nil { + l.Infof("No stored folder metadata for %q; recalculating", folder) + return db.getMetaAndCheck(folder) + } + + curSeq := meta.Sequence(protocol.LocalDeviceID) + if metaOK := db.verifyLocalSequence(curSeq, folder); !metaOK { + l.Infof("Stored folder metadata for %q is out of date after crash; recalculating", folder) + return db.getMetaAndCheck(folder) + } + + if age := time.Since(meta.Created()); age > databaseRecheckInterval { + l.Infof("Stored folder metadata for %q is %v old; recalculating", folder, age) + return db.getMetaAndCheck(folder) + } + + return meta +} + +func (db *Lowlevel) recalcMeta(folder string) (*metadataTracker, error) { + meta := newMetadataTracker() + if err := db.checkGlobals([]byte(folder), meta); err != nil { + return nil, err + } + + t, err := db.newReadWriteTransaction() + if err != nil { + return nil, err + } + defer t.close() + + var deviceID protocol.DeviceID + err = t.withAllFolderTruncated([]byte(folder), func(device []byte, f FileInfoTruncated) bool { + copy(deviceID[:], device) + meta.addFile(deviceID, f) + return true + }) + if err != nil { + return nil, err + } + + meta.SetCreated() + if err := meta.toDB(t, []byte(folder)); err != nil { + return nil, err + } + if err := t.Commit(); err != nil { + return nil, err + } + return meta, nil +} + +// Verify the local sequence number from actual sequence entries. Returns +// true if it was all good, or false if a fixup was necessary. +func (db *Lowlevel) verifyLocalSequence(curSeq int64, folder string) bool { + // Walk the sequence index from the current (supposedly) highest + // sequence number and raise the alarm if we get anything. This recovers + // from the occasion where we have written sequence entries to disk but + // not yet written new metadata to disk. + // + // Note that we can have the same thing happen for remote devices but + // there it's not a problem -- we'll simply advertise a lower sequence + // number than we've actually seen and receive some duplicate updates + // and then be in sync again. + + t, err := db.newReadOnlyTransaction() + if err != nil { + panic(err) + } + ok := true + if err := t.withHaveSequence([]byte(folder), curSeq+1, func(fi FileIntf) bool { + ok = false // we got something, which we should not have + return false + }); err != nil && !backend.IsClosed(err) { + panic(err) + } + t.close() + + return ok +} + // repairSequenceGCLocked makes sure the sequence numbers in the sequence keys // match those in the corresponding file entries. It returns the amount of fixed // entries. diff --git a/lib/db/set.go b/lib/db/set.go index d5e38da97..226bb1527 100644 --- a/lib/db/set.go +++ b/lib/db/set.go @@ -75,112 +75,11 @@ func NewFileSet(folder string, fs fs.Filesystem, db *Lowlevel) *FileSet { folder: folder, fs: fs, db: db, - meta: loadMetadataTracker(db, folder), + meta: db.loadMetadataTracker(folder), updateMutex: sync.NewMutex(), } } -func loadMetadataTracker(db *Lowlevel, folder string) *metadataTracker { - recalc := func() *metadataTracker { - db.gcMut.RLock() - defer db.gcMut.RUnlock() - meta, err := recalcMeta(db, folder) - if err == nil { - var fixed int - fixed, err = db.repairSequenceGCLocked(folder, meta) - if fixed != 0 { - l.Infoln("Repaired %v sequence entries in database", fixed) - } - } - if backend.IsClosed(err) { - return nil - } else if err != nil { - panic(err) - } - return meta - } - - meta := newMetadataTracker() - if err := meta.fromDB(db, []byte(folder)); err != nil { - l.Infof("No stored folder metadata for %q; recalculating", folder) - return recalc() - } - - curSeq := meta.Sequence(protocol.LocalDeviceID) - if metaOK := verifyLocalSequence(curSeq, db, folder); !metaOK { - l.Infof("Stored folder metadata for %q is out of date after crash; recalculating", folder) - return recalc() - } - - if age := time.Since(meta.Created()); age > databaseRecheckInterval { - l.Infof("Stored folder metadata for %q is %v old; recalculating", folder, age) - return recalc() - } - - return meta -} - -func recalcMeta(db *Lowlevel, folder string) (*metadataTracker, error) { - meta := newMetadataTracker() - if err := db.checkGlobals([]byte(folder), meta); err != nil { - return nil, err - } - - t, err := db.newReadWriteTransaction() - if err != nil { - return nil, err - } - defer t.close() - - var deviceID protocol.DeviceID - err = t.withAllFolderTruncated([]byte(folder), func(device []byte, f FileInfoTruncated) bool { - copy(deviceID[:], device) - meta.addFile(deviceID, f) - return true - }) - if err != nil { - return nil, err - } - - meta.SetCreated() - if err := meta.toDB(t, []byte(folder)); err != nil { - return nil, err - } - if err := t.Commit(); err != nil { - return nil, err - } - return meta, nil -} - -// Verify the local sequence number from actual sequence entries. Returns -// true if it was all good, or false if a fixup was necessary. -func verifyLocalSequence(curSeq int64, db *Lowlevel, folder string) bool { - // Walk the sequence index from the current (supposedly) highest - // sequence number and raise the alarm if we get anything. This recovers - // from the occasion where we have written sequence entries to disk but - // not yet written new metadata to disk. - // - // Note that we can have the same thing happen for remote devices but - // there it's not a problem -- we'll simply advertise a lower sequence - // number than we've actually seen and receive some duplicate updates - // and then be in sync again. - - t, err := db.newReadOnlyTransaction() - if err != nil { - panic(err) - } - ok := true - if err := t.withHaveSequence([]byte(folder), curSeq+1, func(fi FileIntf) bool { - ok = false // we got something, which we should not have - return false - }); err != nil && !backend.IsClosed(err) { - panic(err) - } - t.close() - - return ok -} - func (s *FileSet) Drop(device protocol.DeviceID) { l.Debugf("%s Drop(%v)", s.folder, device) diff --git a/lib/syncthing/syncthing.go b/lib/syncthing/syncthing.go index 3bce1f962..15faed57f 100644 --- a/lib/syncthing/syncthing.go +++ b/lib/syncthing/syncthing.go @@ -35,6 +35,7 @@ import ( "github.com/syncthing/syncthing/lib/rand" "github.com/syncthing/syncthing/lib/sha256" "github.com/syncthing/syncthing/lib/tlsutil" + "github.com/syncthing/syncthing/lib/upgrade" "github.com/syncthing/syncthing/lib/ur" ) @@ -224,7 +225,7 @@ func (a *App) startup() error { prevParts := strings.Split(prevVersion, "-") curParts := strings.Split(build.Version, "-") - if prevParts[0] != curParts[0] { + if rel := upgrade.CompareVersions(prevParts[0], curParts[0]); rel != upgrade.Equal { if prevVersion != "" { l.Infoln("Detected upgrade from", prevVersion, "to", build.Version) } @@ -237,6 +238,14 @@ func (a *App) startup() error { miscDB.PutString("prevVersion", build.Version) } + // Check and repair metadata and sequences on every upgrade including RCs. + prevParts = strings.Split(prevVersion, "+") + curParts = strings.Split(build.Version, "+") + if rel := upgrade.CompareVersions(prevParts[0], curParts[0]); rel != upgrade.Equal { + l.Infoln("Checking db due to upgrade - this may take a while...") + a.ll.CheckRepair() + } + m := model.NewModel(a.cfg, a.myID, "syncthing", build.Version, a.ll, protectedFiles, a.evLogger) if a.opts.DeadlockTimeoutS > 0 {