Repository: syncthing/syncthing (language: go)

Issue:
All devices shown as not having accepted shared folders
The visual indication added in https://github.com/syncthing/syncthing/pull/8201 is active for all of my folders (old long accepted, and newly shared ones). @acolomb Can you repro?

Added skip-changelog, as it's only released as nightlies so far.


Discussion / hints:
Are the devices currently disconnected or paused by any chance? If so, see the Caveats headline in the PR description. Guess there was some unfinished business after all. The current default value was suggested like that by @calmh in the forum thread, but I think it makes more sense to change it and not mark disconnected devices as having unaccepted folders.

If that's not the cause for your issue here, let me investigate when back at my desk tonight. 
Not just disconnected, literally all of them.

For the general point about indication and disconnected devices: I'd agree with not showing any information by default. It's an auxiliary information, so having a potential false negative on disconnected devices seems less problematic (consuing) than showing folders as not accepted e.g. on a very old folder-device combo.
I'm testing this right now, and it seems that the wrong status somehow "fixes" itself with time. For instance, right after sharing a folder from Device 1 with Device 2 and accepting it on the latter, the status on Device 1 still said that the folder hadn't been accepted. However, I then left Syncthing running for ~40 minutes, and now there's no "not having accepted" notification anymore, on neither of the two devices.

Obviously, the expected behaviour would be to immediately clear the notification as soon as the other device has accepted sharing the folder.
I'm having trouble investigating at all, as the current main branch no longer builds with my installed Go version 1.16.  Haven't found a new deb package yet, as https://launchpad.net/ubuntu/+source/golang-1.17 is not available for 20.04...  Shoosh!
I have no good explanation for the delays.  Could imagine that after your 40 minutes, the connection gets replaced and therefore a new ClusterConfig is processed, fixing the previously wrong entry.  But ClusterConfig is also sent when the set of shared folders changes, so it should happen immediately.  I cannot reproduce this on my end.

Anyway, making `remoteValid` the default value fixes the display for paused and disconnected devices in my testing.

Can either of you dig into where this happens in the GUI code, respectively what the `/rest/db/completion` API responds at the time?
This is what I've been doing.

1. Start two new Syncthing instances.
2. Connect them with each other.
3. Create a new folder on syncthing1, then share it with syncthing2.
4. Accept the folder on syncthing2.

The situation looks like this in the GUI.

![image](https://user-images.githubusercontent.com/5626656/162838622-80834525-0607-4c90-90c4-ee72a0238e22.png)
![image](https://user-images.githubusercontent.com/5626656/162838638-ca427e44-32eb-430b-9589-126f6ac6a0df.png)

The output of `/rest/db/completion` is

```
{
  "accepted": false,
  "completion": 100,
  "globalBytes": 0,
  "globalItems": 0,
  "needBytes": 0,
  "needDeletes": 0,
  "needItems": 0,
  "sequence": 0
}
```

and it's identical on both sides.

**Edit**: Refreshing the Web GUI in the browser **manually** gets rid of the notification. This means the GUI doesn't refresh itself when it should. I've done all my testing using Chromium only, so I've got no idea at the moment whether the problem is limited to the single browser or not.
I suppose this might be the culprit:

    func (c *folderSummaryService) listenForUpdates(ctx context.Context) error {
	    sub := c.evLogger.Subscribe(events.LocalIndexUpdated | events.RemoteIndexUpdated | events.StateChanged | events.RemoteDownloadProgress | events.DeviceConnected | events.FolderWatchStateChanged | events.DownloadProgress)

There is simply no event that triggers this again after receiving a new ClusterConfig from the remote with the folder now included.  Thus the completion is not recalculated and the state won't change in the GUI.  Refreshing will pull the completion for all folders from the endpoint again, thus fixing the display "manually".

@imsodin does that sound like a sensible explanation?  I guess updating completion when the remote ClusterConfig changes would be a good idea anyway.  But maybe I just don't see where it's already done.
That sounds plausible, yes. Not sure what the nicest way to fix it is: Adding an event on cluster-config just for this seems too heavy handed, then we might as well go back on the original decision of adding it to completions and create a dedicated event for the remote folder state.
What's your recommendation for the RC we should have shipped yesterday? I don't want to ship it with this being non-functional and if there are API changes coming. Temporarily revert and then add it in the next RC? 
For the most part, this will be fixed by #8275 I think.  Let's handle the missing immediate update on ClusterConfig after the RC.  I don't think we need to switch away from the current API again.  I'd rather look for an internal solution that just triggers the summary service in that case.
I confirmed it's fixed by https://github.com/syncthing/syncthing/pull/8275

I still think of that change as just a "hot-fix" to get the RC out with acceptable user-facing behaviour, but I don't want to keep that behaviour. I currently see two independent possible options:

1. Make the default for the remote folder state enum something like `unknown`. Add the state enum to the folder completion instead of an `accepted` boolean. We set that to true at the moment, while we don't actually know that it's true, because that results in the right UI display - that's unnecessary/bad coupling of the model/event/api with UI.
2. Add a dedicated event/endpoint for remote folder status, instead of adding it on the folder completion as we do know. This will also immediately propagate the information when available from a received CC.
Agreed, it's just an intermediate fix until I had some time to work it out. Not sure when, as I'm currently traveling with family, but I do have a laptop in reach.

Regardless of which option we choose, the fact that FolderCompletion is not resent for folders added in the last ClusterConfig is a bug in itself. Fixing that will help here as well if we stay with the current event. 

Fix patch:
diff --git a/gui/default/syncthing/core/eventService.js b/gui/default/syncthing/core/eventService.js
index 7e528d5a38f..cb37b652064 100644
--- a/gui/default/syncthing/core/eventService.js
+++ b/gui/default/syncthing/core/eventService.js
@@ -64,6 +64,7 @@ angular.module('syncthing.core')
             PENDING_DEVICES_CHANGED: 'PendingDevicesChanged',   // Emitted when pending devices were added / updated (connection from unknown ID) or removed (device is ignored or added)
             DEVICE_PAUSED: 'DevicePaused',   // Emitted when a device has been paused
             DEVICE_RESUMED: 'DeviceResumed',   // Emitted when a device has been resumed
+            CLUSTER_CONFIG_RECEIVED: 'ClusterConfigReceived',   // Emitted when receiving a remote device's cluster config
             DOWNLOAD_PROGRESS: 'DownloadProgress',   // Emitted during file downloads for each folder for each file
             FAILURE: 'Failure',   // Specific errors sent to the usage reporting server for diagnosis
             FOLDER_COMPLETION: 'FolderCompletion',   //Emitted when the local or remote contents for a folder changes
diff --git a/gui/default/syncthing/core/syncthingController.js b/gui/default/syncthing/core/syncthingController.js
index 7e72a67b671..939ff585967 100755
--- a/gui/default/syncthing/core/syncthingController.js
+++ b/gui/default/syncthing/core/syncthingController.js
@@ -2368,7 +2368,7 @@ angular.module('syncthing.core')
         $scope.deviceNameMarkUnaccepted = function (deviceID, folderID) {
             var name = $scope.deviceName($scope.devices[deviceID]);
             // Add footnote if sharing was not accepted on the remote device
-            if (deviceID in $scope.completion && folderID in $scope.completion[deviceID] && !$scope.completion[deviceID][folderID].accepted) {
+            if (deviceID in $scope.completion && folderID in $scope.completion[deviceID] && $scope.completion[deviceID][folderID].remoteState == 'notSharing') {
                 name += '<sup>1</sup>';
             }
             return name;
@@ -2389,7 +2389,7 @@ angular.module('syncthing.core')
             for (var deviceID in $scope.completion) {
                 if (deviceID in $scope.devices
                     && folderCfg.id in $scope.completion[deviceID]
-                    && !$scope.completion[deviceID][folderCfg.id].accepted) {
+                    && $scope.completion[deviceID][folderCfg.id].remoteState == 'notSharing') {
                     return true;
                 }
             }
@@ -2420,7 +2420,7 @@ angular.module('syncthing.core')
         $scope.folderLabelMarkUnaccepted = function (folderID, deviceID) {
             var label = $scope.folderLabel(folderID);
             // Add footnote if sharing was not accepted on the remote device
-            if (deviceID in $scope.completion && folderID in $scope.completion[deviceID] && !$scope.completion[deviceID][folderID].accepted) {
+            if (deviceID in $scope.completion && folderID in $scope.completion[deviceID] && $scope.completion[deviceID][folderID].remoteState == 'notSharing') {
                 label += '<sup>1</sup>';
             }
             return label;
@@ -2440,7 +2440,7 @@ angular.module('syncthing.core')
             }
             for (var folderID in $scope.completion[deviceCfg.deviceID]) {
                 if (folderID in $scope.folders
-                    && !$scope.completion[deviceCfg.deviceID][folderID].accepted) {
+                    && $scope.completion[deviceCfg.deviceID][folderID].remoteState == 'notSharing') {
                     return true;
                 }
             }
diff --git a/lib/events/events.go b/lib/events/events.go
index 53976f59ad4..20b34795577 100644
--- a/lib/events/events.go
+++ b/lib/events/events.go
@@ -35,6 +35,7 @@ const (
 	PendingDevicesChanged
 	DevicePaused
 	DeviceResumed
+	ClusterConfigReceived
 	LocalChangeDetected
 	RemoteChangeDetected
 	LocalIndexUpdated
@@ -118,6 +119,8 @@ func (t EventType) String() string {
 		return "DevicePaused"
 	case DeviceResumed:
 		return "DeviceResumed"
+	case ClusterConfigReceived:
+		return "ClusterConfigReceived"
 	case FolderScanProgress:
 		return "FolderScanProgress"
 	case FolderPaused:
@@ -203,6 +206,8 @@ func UnmarshalEventType(s string) EventType {
 		return DevicePaused
 	case "DeviceResumed":
 		return DeviceResumed
+	case "ClusterConfigReceived":
+		return ClusterConfigReceived
 	case "FolderScanProgress":
 		return FolderScanProgress
 	case "FolderPaused":
diff --git a/lib/model/folder_summary.go b/lib/model/folder_summary.go
index 405a8e54bc9..b3c92447614 100644
--- a/lib/model/folder_summary.go
+++ b/lib/model/folder_summary.go
@@ -221,7 +221,7 @@ func (c *folderSummaryService) OnEventRequest() {
 // listenForUpdates subscribes to the event bus and makes note of folders that
 // need their data recalculated.
 func (c *folderSummaryService) listenForUpdates(ctx context.Context) error {
-	sub := c.evLogger.Subscribe(events.LocalIndexUpdated | events.RemoteIndexUpdated | events.StateChanged | events.RemoteDownloadProgress | events.DeviceConnected | events.FolderWatchStateChanged | events.DownloadProgress)
+	sub := c.evLogger.Subscribe(events.LocalIndexUpdated | events.RemoteIndexUpdated | events.StateChanged | events.RemoteDownloadProgress | events.DeviceConnected | events.ClusterConfigReceived | events.FolderWatchStateChanged | events.DownloadProgress)
 	defer sub.Unsubscribe()
 
 	for {
@@ -244,12 +244,18 @@ func (c *folderSummaryService) processUpdate(ev events.Event) {
 	var folder string
 
 	switch ev.Type {
-	case events.DeviceConnected:
+	case events.DeviceConnected, events.ClusterConfigReceived:
 		// When a device connects we schedule a refresh of all
 		// folders shared with that device.
 
-		data := ev.Data.(map[string]string)
-		deviceID, _ := protocol.DeviceIDFromString(data["id"])
+		var deviceID protocol.DeviceID
+		if ev.Type == events.DeviceConnected {
+			data := ev.Data.(map[string]string)
+			deviceID, _ = protocol.DeviceIDFromString(data["id"])
+		} else {
+			data := ev.Data.(ClusterConfigReceivedEventData)
+			deviceID = data.Device
+		}
 
 		c.foldersMut.Lock()
 	nextFolder:
diff --git a/lib/model/folderstate.go b/lib/model/folderstate.go
index 897875c37c0..0437e259a4a 100644
--- a/lib/model/folderstate.go
+++ b/lib/model/folderstate.go
@@ -55,11 +55,31 @@ func (s folderState) String() string {
 type remoteFolderState int
 
 const (
-	remoteValid remoteFolderState = iota
-	remoteNotSharing
-	remotePaused
+	remoteFolderUnknown remoteFolderState = iota
+	remoteFolderNotSharing
+	remoteFolderPaused
+	remoteFolderValid
 )
 
+func (s remoteFolderState) String() string {
+	switch s {
+	case remoteFolderUnknown:
+		return "unknown"
+	case remoteFolderNotSharing:
+		return "notSharing"
+	case remoteFolderPaused:
+		return "paused"
+	case remoteFolderValid:
+		return "valid"
+	default:
+		return "unknown"
+	}
+}
+
+func (s remoteFolderState) MarshalText() ([]byte, error) {
+	return []byte(s.String()), nil
+}
+
 type stateTracker struct {
 	folderID string
 	evLogger events.Logger
diff --git a/lib/model/model.go b/lib/model/model.go
index 5391109e725..0aa5299858c 100644
--- a/lib/model/model.go
+++ b/lib/model/model.go
@@ -800,10 +800,10 @@ type FolderCompletion struct {
 	NeedItems     int
 	NeedDeletes   int
 	Sequence      int64
-	Accepted      bool
+	RemoteState   remoteFolderState
 }
 
-func newFolderCompletion(global, need db.Counts, sequence int64, accepted bool) FolderCompletion {
+func newFolderCompletion(global, need db.Counts, sequence int64, state remoteFolderState) FolderCompletion {
 	comp := FolderCompletion{
 		GlobalBytes: global.Bytes,
 		NeedBytes:   need.Bytes,
@@ -811,7 +811,7 @@ func newFolderCompletion(global, need db.Counts, sequence int64, accepted bool)
 		NeedItems:   need.Files + need.Directories + need.Symlinks,
 		NeedDeletes: need.Deleted,
 		Sequence:    sequence,
-		Accepted:    accepted,
+		RemoteState: state,
 	}
 	comp.setComplectionPct()
 	return comp
@@ -843,7 +843,7 @@ func (comp *FolderCompletion) setComplectionPct() {
 	}
 }
 
-// Map returns the members as a map, e.g. used in api to serialize as Json.
+// Map returns the members as a map, e.g. used in api to serialize as JSON.
 func (comp FolderCompletion) Map() map[string]interface{} {
 	return map[string]interface{}{
 		"completion":  comp.CompletionPct,
@@ -853,7 +853,7 @@ func (comp FolderCompletion) Map() map[string]interface{} {
 		"needItems":   comp.NeedItems,
 		"needDeletes": comp.NeedDeletes,
 		"sequence":    comp.Sequence,
-		"accepted":    comp.Accepted,
+		"remoteState": comp.RemoteState,
 	}
 }
 
@@ -904,7 +904,7 @@ func (m *model) folderCompletion(device protocol.DeviceID, folder string) (Folde
 	defer snap.Release()
 
 	m.pmut.RLock()
-	accepted := m.remoteFolderStates[device][folder] != remoteNotSharing
+	state := m.remoteFolderStates[device][folder]
 	downloaded := m.deviceDownloads[device].BytesDownloaded(folder)
 	m.pmut.RUnlock()
 
@@ -915,7 +915,7 @@ func (m *model) folderCompletion(device protocol.DeviceID, folder string) (Folde
 		need.Bytes = 0
 	}
 
-	comp := newFolderCompletion(snap.GlobalSize(), need, snap.Sequence(device), accepted)
+	comp := newFolderCompletion(snap.GlobalSize(), need, snap.Sequence(device), state)
 
 	l.Debugf("%v Completion(%s, %q): %v", m, device, folder, comp.Map())
 	return comp, nil
@@ -1147,6 +1147,10 @@ type clusterConfigDeviceInfo struct {
 	local, remote protocol.Device
 }
 
+type ClusterConfigReceivedEventData struct {
+	Device protocol.DeviceID `json:"device"`
+}
+
 func (m *model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterConfig) error {
 	// Check the peer device's announced folders against our own. Emits events
 	// for folders that we don't expect (unknown or not shared).
@@ -1234,6 +1238,10 @@ func (m *model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterCon
 	m.remoteFolderStates[deviceID] = states
 	m.pmut.Unlock()
 
+	m.evLogger.Log(events.ClusterConfigReceived, ClusterConfigReceivedEventData{
+		Device: deviceID,
+	})
+
 	if len(tempIndexFolders) > 0 {
 		m.pmut.RLock()
 		conn, ok := m.conn[deviceID]
@@ -1278,7 +1286,7 @@ func (m *model) ccHandleFolders(folders []protocol.Folder, deviceCfg config.Devi
 	}
 	of := db.ObservedFolder{Time: time.Now().Truncate(time.Second)}
 	for _, folder := range folders {
-		seenFolders[folder.ID] = remoteValid
+		seenFolders[folder.ID] = remoteFolderValid
 
 		cfg, ok := m.cfg.Folder(folder.ID)
 		if ok {
@@ -1319,7 +1327,7 @@ func (m *model) ccHandleFolders(folders []protocol.Folder, deviceCfg config.Devi
 
 		if folder.Paused {
 			indexHandlers.Remove(folder.ID)
-			seenFolders[cfg.ID] = remotePaused
+			seenFolders[cfg.ID] = remoteFolderPaused
 			continue
 		}
 
@@ -1374,8 +1382,8 @@ func (m *model) ccHandleFolders(folders []protocol.Folder, deviceCfg config.Devi
 	// Explicitly mark folders we offer, but the remote has not accepted
 	for folderID, cfg := range m.cfg.Folders() {
 		if _, seen := seenFolders[folderID]; !seen && cfg.SharedWith(deviceID) {
-			l.Debugf("Remote device %v has not accepted folder %s", deviceID.Short(), cfg.Description())
-			seenFolders[folderID] = remoteNotSharing
+			l.Debugf("Remote device %v has not accepted sharing folder %s", deviceID.Short(), cfg.Description())
+			seenFolders[folderID] = remoteFolderNotSharing
 		}
 	}
 
@@ -2712,7 +2720,7 @@ func (m *model) availabilityInSnapshotPRlocked(cfg config.FolderConfiguration, s
 		if _, ok := m.remoteFolderStates[device]; !ok {
 			continue
 		}
-		if state, ok := m.remoteFolderStates[device][cfg.ID]; !ok || state == remotePaused {
+		if state := m.remoteFolderStates[device][cfg.ID]; state != remoteFolderValid {
 			continue
 		}
 		_, ok := m.conn[device]
diff --git a/lib/syncthing/verboseservice.go b/lib/syncthing/verboseservice.go
index 8d7084ad0ed..cea602d3f44 100644
--- a/lib/syncthing/verboseservice.go
+++ b/lib/syncthing/verboseservice.go
@@ -117,7 +117,7 @@ func (s *verboseService) formatEvent(ev events.Event) string {
 
 	case events.FolderCompletion:
 		data := ev.Data.(map[string]interface{})
-		return fmt.Sprintf("Completion for folder %q on device %v is %v%% (accepted: %v)", data["folder"], data["device"], data["completion"], data["accepted"])
+		return fmt.Sprintf("Completion for folder %q on device %v is %v%% (state: %s)", data["folder"], data["device"], data["completion"], data["remoteState"])
 
 	case events.FolderSummary:
 		data := ev.Data.(model.FolderSummaryEventData)
@@ -145,6 +145,10 @@ func (s *verboseService) formatEvent(ev events.Event) string {
 		device := data["device"]
 		return fmt.Sprintf("Device %v was resumed", device)
 
+	case events.ClusterConfigReceived:
+		data := ev.Data.(model.ClusterConfigReceivedEventData)
+		return fmt.Sprintf("Received ClusterConfig from device %v", data.Device)
+
 	case events.FolderPaused:
 		data := ev.Data.(map[string]string)
 		id := data["id"]


Test patch:
diff --git a/lib/model/model_test.go b/lib/model/model_test.go
index 6d25a3f193d..39e79dfc74a 100644
--- a/lib/model/model_test.go
+++ b/lib/model/model_test.go
@@ -3764,16 +3764,16 @@ func TestClusterConfigOnFolderUnpause(t *testing.T) {
 
 func TestAddFolderCompletion(t *testing.T) {
 	// Empty folders are always 100% complete.
-	comp := newFolderCompletion(db.Counts{}, db.Counts{}, 0, true)
-	comp.add(newFolderCompletion(db.Counts{}, db.Counts{}, 0, false))
+	comp := newFolderCompletion(db.Counts{}, db.Counts{}, 0, remoteFolderValid)
+	comp.add(newFolderCompletion(db.Counts{}, db.Counts{}, 0, remoteFolderPaused))
 	if comp.CompletionPct != 100 {
 		t.Error(comp.CompletionPct)
 	}
 
 	// Completion is of the whole
-	comp = newFolderCompletion(db.Counts{Bytes: 100}, db.Counts{}, 0, true)             // 100% complete
-	comp.add(newFolderCompletion(db.Counts{Bytes: 400}, db.Counts{Bytes: 50}, 0, true)) // 82.5% complete
-	if comp.CompletionPct != 90 {                                                       // 100 * (1 - 50/500)
+	comp = newFolderCompletion(db.Counts{Bytes: 100}, db.Counts{}, 0, remoteFolderValid)             // 100% complete
+	comp.add(newFolderCompletion(db.Counts{Bytes: 400}, db.Counts{Bytes: 50}, 0, remoteFolderValid)) // 82.5% complete
+	if comp.CompletionPct != 90 {                                                                    // 100 * (1 - 50/500)
 		t.Error(comp.CompletionPct)
 	}
 }