Parcourir la source

fix(node): stop the offline-sync toast firing on saves to online nodes

IsNodePending fed the user-facing "saved locally, node offline, will
sync on reconnect" toast off three conditions, one of which was the
node's config_dirty flag. But every node-backed client/inbound edit
marks the node dirty unconditionally inside its write transaction — it
is the reconcile self-heal marker, set even for edits pushed live to a
healthy online node. The controller reads that freshly-set flag right
after the save, so the warning fired on every save to a node-backed
inbound regardless of the node actually being online.

Drop the dirty term so the predicate reflects only what the message
claims: the node being unreachable (offline or disabled). Offline and
disabled nodes still mark dirty and still surface the toast.

Add regression tests: online+dirty must not be pending; offline and
disabled must be.
MHSanaei il y a 7 heures
Parent
commit
86813758cc
2 fichiers modifiés avec 62 ajouts et 2 suppressions
  1. 9 2
      internal/web/service/node.go
  2. 53 0
      internal/web/service/node_dirty_test.go

+ 9 - 2
internal/web/service/node.go

@@ -780,12 +780,19 @@ func (s *NodeService) NodeSyncState(id int) (enabled bool, status string, dirty
 	return row.Enable, row.Status, row.ConfigDirty, row.ConfigDirtyAt, nil
 }
 
+// IsNodePending reports whether a save targeting this node was deferred because
+// the node is unreachable right now — offline or disabled — so the edit only
+// reaches it on the next reconcile. It deliberately ignores config_dirty: that
+// flag is set on EVERY node-backed edit as the reconcile self-heal marker,
+// including edits pushed live to an online node, so keying the user-facing
+// "saved, node offline, will sync" toast off it fired the warning on every save
+// to a perfectly healthy online node.
 func (s *NodeService) IsNodePending(id int) bool {
-	enabled, status, dirty, _, err := s.NodeSyncState(id)
+	enabled, status, _, _, err := s.NodeSyncState(id)
 	if err != nil {
 		return false
 	}
-	return !enabled || status != "online" || dirty
+	return !enabled || status != "online"
 }
 
 func nodeMetricKey(id int, metric string) string {

+ 53 - 0
internal/web/service/node_dirty_test.go

@@ -110,6 +110,59 @@ func TestDelInboundClientByEmail_DisabledNodeClientMarksDirty(t *testing.T) {
 	}
 }
 
+// An online, enabled node that is merely config-dirty must NOT be reported as
+// pending: every node-backed edit marks the node dirty as the reconcile
+// self-heal marker, so keying the "saved, node offline, will sync" toast off
+// the dirty flag fired it on every save to a healthy online node.
+func TestIsNodePending_OnlineDirtyNodeIsNotPending(t *testing.T) {
+	setupConflictDB(t)
+	db := database.GetDB()
+
+	node := &model.Node{Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true, Status: "online"}
+	if err := db.Create(node).Error; err != nil {
+		t.Fatalf("create node: %v", err)
+	}
+
+	nodeSvc := NodeService{}
+	if nodeSvc.IsNodePending(node.Id) {
+		t.Fatal("a clean online node must not be pending")
+	}
+	if err := nodeSvc.MarkNodeDirty(node.Id); err != nil {
+		t.Fatalf("MarkNodeDirty: %v", err)
+	}
+	if nodeSvc.IsNodePending(node.Id) {
+		t.Fatal("an online, enabled node must not be pending just because it is config-dirty")
+	}
+}
+
+// Offline or disabled nodes are genuinely deferred and must report pending so
+// the "saved, node offline, will sync" toast still surfaces for them.
+func TestIsNodePending_OfflineOrDisabledIsPending(t *testing.T) {
+	setupConflictDB(t)
+	db := database.GetDB()
+
+	offline := &model.Node{Name: "off", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true, Status: "offline"}
+	disabled := &model.Node{Name: "dis", Address: "127.0.0.1", Port: 2097, ApiToken: "tok", Enable: false, Status: "online"}
+	for _, n := range []*model.Node{offline, disabled} {
+		if err := db.Create(n).Error; err != nil {
+			t.Fatalf("create node %s: %v", n.Name, err)
+		}
+	}
+	// Node.Enable carries gorm default:true, so Create({Enable:false}) persists
+	// TRUE — force the column off to actually exercise the disabled path.
+	if err := db.Model(&model.Node{}).Where("id = ?", disabled.Id).Update("enable", false).Error; err != nil {
+		t.Fatalf("force-disable node: %v", err)
+	}
+
+	nodeSvc := NodeService{}
+	if !nodeSvc.IsNodePending(offline.Id) {
+		t.Fatal("an offline node must be pending")
+	}
+	if !nodeSvc.IsNodePending(disabled.Id) {
+		t.Fatal("a disabled node must be pending")
+	}
+}
+
 // ClearNodeDirty must be a compare-and-swap on config_dirty_at so a concurrent
 // edit that re-dirties the node during a reconcile is not silently cleared.
 func TestNodeDirty_ClearIsCASOnDirtyAt(t *testing.T) {