fix(model): properly health-check up-to-date folders (fixes #10546) (#10773)

Previously we'd skip the health check for an up-to-date folder, so it
would continue looking up-to-date even when the folder path missing.

---------

Signed-off-by: Jakob Borg <jakob@kastelo.net>

lib/model/folder.go

@@ -195,19 +195,23 @@ func (f *folder) Serve(ctx context.Context) error {
 		case <-f.pullScheduled:
 			if f.PullerDelayS > 0 {
 				// Wait for incoming updates to settle before doing the
-				// actual pull. Only set the state to SyncWaiting if we have
+				// actual pull.
-				// reason to believe there is something to sync, to avoid
+				//
-				// unnecessary flashing in the GUI.
+				// Only set the state to SyncWaiting if we are Idle (and
-				if needCount, err := f.db.CountNeed(f.folderID, protocol.LocalDeviceID); err == nil && needCount.TotalItems() > 0 {
+				// not, e.g., errored) and have reason to believe there is
-					f.setState(FolderSyncWaiting)
+				// something to sync, to avoid unnecessary flashing in the
+				// GUI.
+				if cur, _, _ := f.getState(); cur == FolderIdle {
+					if needCount, err := f.db.CountNeed(f.folderID, protocol.LocalDeviceID); err == nil && needCount.TotalItems() > 0 {
+						f.setState(FolderSyncWaiting)
+					}
 				}
 				pullTimer.Reset(time.Duration(float64(time.Second) * f.PullerDelayS))
-			} else {
+				continue
-				_, err = f.pull(ctx)
 			}
+			_, err = f.pull(ctx)
 
 		case <-pullTimer.C:
-			f.setState(FolderIdle)
 			_, err = f.pull(ctx)
 
 		case <-f.pullFailTimer.C:
@@ -238,10 +242,12 @@ func (f *folder) Serve(ctx context.Context) error {
 		case next := <-f.scanDelay:
 			f.sl.DebugContext(ctx, "Delaying scan")
 			f.scanTimer.Reset(next)
+			continue
 
 		case <-f.scanScheduled:
 			f.sl.DebugContext(ctx, "Scan was scheduled")
 			f.scanTimer.Reset(0)
+			continue
 
 		case fsEvents := <-f.watchChan:
 			f.sl.DebugContext(ctx, "Scan due to watcher")
@@ -252,16 +258,20 @@ func (f *folder) Serve(ctx context.Context) error {
 			err = f.restartWatch(ctx)
 
 		case <-f.versionCleanupTimer.C:
+			if _, _, healthErr := f.getState(); healthErr != nil {
+				continue
+			}
 			f.sl.DebugContext(ctx, "Doing version cleanup")
 			f.versionCleanupTimerFired(ctx)
 		}
 
-		if err != nil {
+		if svcutil.IsFatal(err) {
-			if svcutil.IsFatal(err) {
+			return err
-				return err
-			}
-			f.setError(ctx, err)
 		}
+
+		// Set the state to FolderError when err is non-nil, otherwise reset
+		// it back to FolderIdle.
+		f.setError(ctx, err)
 	}
 }
 
@@ -403,6 +413,15 @@ func (f *folder) pull(ctx context.Context) (success bool, err error) {
 		}
 	}()
 
+	// Abort early (before acquiring a limiter token) if there's a folder
+	// error. This must happen before the "nothing to do" check below so
+	// that up-to-date folders are also periodically health-checked.
+	err = f.getHealthErrorWithoutIgnores()
+	if err != nil {
+		f.sl.DebugContext(ctx, "Skipping pull due to folder error", slogutil.Error(err))
+		return false, err
+	}
+
 	// If there is nothing to do, don't even enter sync-waiting state.
 	needCount, err := f.db.CountNeed(f.folderID, protocol.LocalDeviceID)
 	if err != nil {
@@ -416,13 +435,6 @@ func (f *folder) pull(ctx context.Context) (success bool, err error) {
 		return true, nil
 	}
 
-	// Abort early (before acquiring a token) if there's a folder error
-	err = f.getHealthErrorWithoutIgnores()
-	if err != nil {
-		f.sl.DebugContext(ctx, "Skipping pull due to folder error", slogutil.Error(err))
-		return false, err
-	}
-
 	// Send only folder doesn't do any io, it only checks for out-of-sync
 	// items that differ in metadata and updates those.
 	if f.Type != config.FolderTypeSendOnly {
@@ -487,7 +499,6 @@ func (f *folder) scanSubdirs(ctx context.Context, subDirs []string) error {
 	}()
 
 	f.setState(FolderScanWaiting)
-	defer f.setState(FolderIdle)
 
 	if err := f.ioLimiter.TakeWithContext(ctx, 1); err != nil {
 		return err
@@ -993,7 +1004,6 @@ func (f *folder) scanTimerFired(ctx context.Context) error {
 
 func (f *folder) versionCleanupTimerFired(ctx context.Context) {
 	f.setState(FolderCleanWaiting)
-	defer f.setState(FolderIdle)
 
 	if err := f.ioLimiter.TakeWithContext(ctx, 1); err != nil {
 		return
@@ -1184,8 +1194,13 @@ func (f *folder) setError(ctx context.Context, err error) {
 	default:
 	}
 
-	_, _, oldErr := f.getState()
+	state, _, oldErr := f.getState()
-	if (err != nil && oldErr != nil && oldErr.Error() == err.Error()) || (err == nil && oldErr == nil) {
+	switch {
+	case err != nil && oldErr != nil && oldErr.Error() == err.Error():
+		// The error is the same as the old error, no change is required
+		return
+	case err == nil && oldErr == nil && state == FolderIdle:
+		// No error before or now, and we are already Idle
 		return
 	}
 
@@ -1195,7 +1210,7 @@ func (f *folder) setError(ctx context.Context, err error) {
 		} else {
 			f.sl.InfoContext(ctx, "Folder error changed", slogutil.Error(err), slog.Any("previously", oldErr))
 		}
-	} else {
+	} else if oldErr != nil {
 		f.sl.InfoContext(ctx, "Folder error cleared")
 		f.SchedulePull()
 	}
@@ -1203,7 +1218,7 @@ func (f *folder) setError(ctx context.Context, err error) {
 	if f.FSWatcherEnabled {
 		if err != nil {
 			f.stopWatch()
-		} else {
+		} else if oldErr != nil {
 			f.scheduleWatchRestart()
 		}
 	}

lib/model/folder_recvenc.go

@@ -44,7 +44,6 @@ func (f *receiveEncryptedFolder) revert(ctx context.Context) error {
 	f.sl.InfoContext(ctx, "Reverting unexpected items")
 
 	f.setState(FolderScanning)
-	defer f.setState(FolderIdle)
 
 	batch := NewFileInfoBatch(func(fs []protocol.FileInfo) error {
 		f.updateLocalsFromScanning(fs)

lib/model/folder_recvonly.go

@@ -74,7 +74,6 @@ func (f *receiveOnlyFolder) revert(ctx context.Context) error {
 	f.sl.InfoContext(ctx, "Reverting folder")
 
 	f.setState(FolderScanning)
-	defer f.setState(FolderIdle)
 
 	scanChan := make(chan string)
 	go f.pullScannerRoutine(ctx, scanChan)

lib/model/folder_sendonly.go

@@ -98,7 +98,6 @@ func (f *sendOnlyFolder) override(ctx context.Context) error {
 	f.sl.InfoContext(ctx, "Overriding global state ")
 
 	f.setState(FolderScanning)
-	defer f.setState(FolderIdle)
 
 	batch := NewFileInfoBatch(func(files []protocol.FileInfo) error {
 		f.updateLocalsFromScanning(files)

lib/model/folder_sendrecv.go

@@ -168,7 +168,6 @@ func (f *sendReceiveFolder) pull(ctx context.Context) (bool, error) {
 	go f.pullScannerRoutine(ctx, scanChan)
 	defer func() {
 		close(scanChan)
-		f.setState(FolderIdle)
 	}()
 
 	metricFolderPulls.WithLabelValues(f.ID).Inc()