lib/db: Fix accounting bug when dropping indexes (#7774)

lib/db/schemaupdater.go

@@ -20,7 +20,7 @@ import (
 // do not put restrictions on downgrades (e.g. for repairs after a bugfix).
 const (
 	dbVersion             = 14
-	dbMigrationVersion    = 18
+	dbMigrationVersion    = 19
 	dbMinSyncthingVersion = "v1.9.0"
 )
 
@@ -102,7 +102,7 @@ func (db *schemaUpdater) updateSchema() error {
 		{14, 14, "v1.9.0", db.updateSchemaTo14},
 		{14, 16, "v1.9.0", db.checkRepairMigration},
 		{14, 17, "v1.9.0", db.migration17},
-		{14, 18, "v1.9.0", db.dropIndexIDsMigration},
+		{14, 19, "v1.9.0", db.dropIndexIDsMigration},
 	}
 
 	for _, m := range migrations {

lib/db/set_test.go

@@ -1784,6 +1784,41 @@ func TestConcurrentIndexID(t *testing.T) {
 	}
 }
 
+func TestNeedRemoveLastValid(t *testing.T) {
+	db := newLowlevelMemory(t)
+	defer db.Close()
+
+	folder := "test"
+	testFs := fs.NewFilesystem(fs.FilesystemTypeFake, "")
+
+	fs := newFileSet(t, folder, testFs, db)
+
+	files := []protocol.FileInfo{
+		{Name: "foo", Version: protocol.Vector{}.Update(myID), Sequence: 1},
+	}
+	fs.Update(remoteDevice0, files)
+	files[0].Version = files[0].Version.Update(myID)
+	fs.Update(remoteDevice1, files)
+	files[0].LocalFlags = protocol.FlagLocalIgnored
+	fs.Update(protocol.LocalDeviceID, files)
+
+	snap := snapshot(t, fs)
+	c := snap.NeedSize(remoteDevice0)
+	if c.Files != 1 {
+		t.Errorf("Expected 1 needed files initially, got %v", c.Files)
+	}
+	snap.Release()
+
+	fs.Drop(remoteDevice1)
+
+	snap = snapshot(t, fs)
+	c = snap.NeedSize(remoteDevice0)
+	if c.Files != 0 {
+		t.Errorf("Expected no needed files, got %v", c.Files)
+	}
+	snap.Release()
+}
+
 func replace(fs *db.FileSet, device protocol.DeviceID, files []protocol.FileInfo) {
 	fs.Drop(device)
 	fs.Update(device, files)

lib/db/structs.go

@@ -331,15 +331,12 @@ func (vl *VersionList) pop(device, name []byte) (FileVersion, bool, bool, error)
 	oldFV := vl.RawVersions[i].copy()
 	if invDevice {
 		vl.RawVersions[i].InvalidDevices = popDeviceAt(vl.RawVersions[i].InvalidDevices, j)
-	} else {
-		vl.RawVersions[i].Devices = popDeviceAt(vl.RawVersions[i].Devices, j)
+		return oldFV, true, false, nil
 	}
+	vl.RawVersions[i].Devices = popDeviceAt(vl.RawVersions[i].Devices, j)
 	// If the last valid device of the previous global was removed above,
-	// the next entry is now the global entry (unless all entries are invalid).
-	if len(vl.RawVersions[i].Devices) == 0 && globalPos == i {
-		return oldFV, true, globalPos == vl.findGlobal(), nil
-	}
-	return oldFV, true, false, nil
+	// the global changed.
+	return oldFV, true, len(vl.RawVersions[i].Devices) == 0 && globalPos == i, nil
 }
 
 // Get returns a FileVersion that contains the given device and whether it has

lib/db/transactions.go

@@ -659,10 +659,7 @@ func (t readWriteTransaction) updateGlobal(gk, keyBuf, folder, device []byte, fi
 	// Must happen before updating global meta: If this is the first
 	// item from this device, it will be initialized with the global state.
 
-	needBefore := false
-	if haveOldGlobal {
-		needBefore = Need(oldGlobalFV, haveRemoved, removedFV.Version)
-	}
+	needBefore := haveOldGlobal && Need(oldGlobalFV, haveRemoved, removedFV.Version)
 	needNow := Need(globalFV, true, file.Version)
 	if needBefore {
 		if keyBuf, oldGlobal, err = t.getGlobalFromFileVersion(keyBuf, folder, name, true, oldGlobalFV); err != nil {
@@ -677,7 +674,8 @@ func (t readWriteTransaction) updateGlobal(gk, keyBuf, folder, device []byte, fi
 		}
 	}
 	if needNow {
-		if keyBuf, global, err = t.updateGlobalGetGlobal(keyBuf, folder, name, file, globalFV); err != nil {
+		keyBuf, global, err = t.getGlobalFromFileVersion(keyBuf, folder, name, true, globalFV)
+		if err != nil {
 			return nil, false, err
 		}
 		gotGlobal = true
@@ -711,8 +709,11 @@ func (t readWriteTransaction) updateGlobal(gk, keyBuf, folder, device []byte, fi
 
 	// Add the new global to the global size counter
 	if !gotGlobal {
-		if keyBuf, global, err = t.updateGlobalGetGlobal(keyBuf, folder, name, file, globalFV); err != nil {
-			return nil, false, err
+		if globalFV.Version.Equal(file.Version) {
+			// The inserted file is the global file
+			global = file
+		} else {
+			keyBuf, global, err = t.getGlobalFromFileVersion(keyBuf, folder, name, true, globalFV)
 		}
 		gotGlobal = true
 	}
@@ -721,10 +722,7 @@ func (t readWriteTransaction) updateGlobal(gk, keyBuf, folder, device []byte, fi
 	// check for local (if not already done before)
 	if !bytes.Equal(device, protocol.LocalDeviceID[:]) {
 		localFV, haveLocal := fl.Get(protocol.LocalDeviceID[:])
-		needBefore := false
-		if haveOldGlobal {
-			needBefore = Need(oldGlobalFV, haveLocal, localFV.Version)
-		}
+		needBefore := haveOldGlobal && Need(oldGlobalFV, haveLocal, localFV.Version)
 		needNow := Need(globalFV, haveLocal, localFV.Version)
 		if needBefore {
 			meta.removeNeeded(protocol.LocalDeviceID, oldGlobal)
@@ -761,14 +759,6 @@ func (t readWriteTransaction) updateGlobal(gk, keyBuf, folder, device []byte, fi
 	return keyBuf, true, nil
 }
 
-func (t readWriteTransaction) updateGlobalGetGlobal(keyBuf, folder, name []byte, file protocol.FileInfo, fv FileVersion) ([]byte, protocol.FileIntf, error) {
-	if fv.Version.Equal(file.Version) {
-		// Inserted a new newest version
-		return keyBuf, file, nil
-	}
-	return t.getGlobalFromFileVersion(keyBuf, folder, name, true, fv)
-}
-
 func (t readWriteTransaction) updateLocalNeed(keyBuf, folder, name []byte, add bool) ([]byte, error) {
 	var err error
 	keyBuf, err = t.keyer.GenerateNeedFileKey(keyBuf, folder, name)
@@ -824,6 +814,7 @@ func (t readWriteTransaction) removeFromGlobal(gk, keyBuf, folder, device, file
 	}
 
 	oldGlobalFV, haveOldGlobal := fl.GetGlobal()
+	oldGlobalFV = oldGlobalFV.copy()
 
 	if !haveOldGlobal {
 		// Shouldn't ever happen, but doesn't hurt to handle.