Drop redundant healthcheck

Signed-off-by: Ettore Di Giacinto <mudler@localai.io>

Author: mudler

core/services/nodes/health.go

@@ -103,6 +103,13 @@ func (hm *HealthMonitor) checkAll(ctx context.Context) {
 }
 
 // doCheckAll performs the actual health check logic for all nodes.
+// Node liveness is determined by heartbeat freshness — both backend and agent
+// workers send periodic HTTP heartbeats to the frontend, so a stale heartbeat
+// means the worker supervisor is down. This is simpler and more reliable than
+// probing individual gRPC backend processes (which can crash independently).
+//
+// Per-model health checks (opt-in) separately probe each model's gRPC address
+// and remove stale model records without affecting the node's overall status.
 func (hm *HealthMonitor) doCheckAll(ctx context.Context) {
 	nodes, err := hm.registry.List(ctx)
 	if err != nil {
@@ -115,12 +122,12 @@ func (hm *HealthMonitor) doCheckAll(ctx context.Context) {
 			continue
 		}
 
-		// Check heartbeat staleness first
+		// Node liveness: heartbeat staleness check.
+		// Workers (both backend and agent) send HTTP heartbeats to the frontend.
+		// If the heartbeat is stale, the worker is presumed down.
 		if time.Since(node.LastHeartbeat) > hm.staleThreshold {
 			xlog.Warn("Node heartbeat stale", "node", node.Name, "lastHeartbeat", node.LastHeartbeat)
 			if hm.autoOffline {
-				// Mark offline instead of deregistering — preserves the node row
-				// so re-registration restores the previous approval status
 				xlog.Info("Marking stale node offline", "node", node.Name)
 				if err := hm.registry.MarkOffline(ctx, node.ID); err != nil {
 					xlog.Error("Failed to mark stale node offline", "node", node.Name, "error", err)
@@ -131,44 +138,19 @@ func (hm *HealthMonitor) doCheckAll(ctx context.Context) {
 			continue
 		}
 
-		// Only gRPC health-check nodes that have models loaded.
-		// Idle nodes (no models) haven't started their gRPC process yet
-		// in NATS mode — connection refused is expected, not a failure.
-		// Heartbeats alone are sufficient to prove the supervisor is alive.
-		models, _ := hm.registry.GetNodeModels(ctx, node.ID)
-		if len(models) == 0 {
-			continue
-		}
-
-		client := hm.clientFactory.NewClient(node.Address, false)
-		checkCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
-		alive, err := client.HealthCheck(checkCtx)
-		cancel()
-
-		if !alive || err != nil {
-			xlog.Warn("Node health check failed", "node", node.Name, "address", node.Address, "error", err)
-			hm.registry.MarkUnhealthy(ctx, node.ID)
-			if closer, ok := client.(io.Closer); ok {
-				closer.Close()
-			}
-			continue
-		}
-
-		// Close the node-level gRPC client now that we're done with it
-		if closer, ok := client.(io.Closer); ok {
-			closer.Close()
-		}
-
+		// Heartbeat is fresh — node is alive
 		if node.Status == StatusUnhealthy {
-			// Node recovered
 			xlog.Info("Node recovered", "node", node.Name)
 			if err := hm.registry.MarkHealthy(ctx, node.ID); err != nil {
 				xlog.Error("Failed to mark node healthy", "node", node.Name, "error", err)
 			}
 		}
 
-		// Per-model backend health check: probe each model's distinct gRPC address
+		// Per-model backend health check (opt-in): probe each model's gRPC address
+		// and remove stale model records. This does NOT affect the node's status —
+		// a crashed backend process is a model-level issue, not a node-level one.
 		if hm.perModelHealthCheck {
+			models, _ := hm.registry.GetNodeModels(ctx, node.ID)
 			for _, m := range models {
 				if m.Address == "" || m.Address == node.Address {
 					continue

core/services/nodes/health_mock_test.go

@@ -275,6 +275,13 @@ func makeTestNode(id, name, address string, status string, lastHeartbeat time.Ti
 	}
 }
 
+// makeTestNodeWithHTTP creates a BackendNode with HTTPAddress set
+func makeTestNodeWithHTTP(id, name, address, httpAddress string, status string, lastHeartbeat time.Time) *BackendNode {
+	n := makeTestNode(id, name, address, status, lastHeartbeat)
+	n.HTTPAddress = httpAddress
+	return n
+}
+
 // helper to build a HealthMonitor with fakes
 func newTestHealthMonitor(store NodeHealthStore, factory BackendClientFactory, autoOffline bool, staleThreshold time.Duration) *HealthMonitor {
 	return &HealthMonitor{

core/services/nodes/health_test.go

@@ -95,7 +95,7 @@ var _ = Describe("HealthMonitor", func() {
 			Expect(fetched.Status).To(Equal(StatusHealthy))
 		})
 
-		It("recovers unhealthy node when gRPC check succeeds", func() {
+		It("recovers unhealthy node when heartbeat is fresh", func() {
 			node := makeNode("unhealthy-worker", "10.0.0.5:50051", 8_000_000_000)
 			Expect(registry.Register(context.Background(), node, true)).To(Succeed())
 			Expect(node.Status).To(Equal(StatusHealthy))
@@ -106,15 +106,11 @@ var _ = Describe("HealthMonitor", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(fetched.Status).To(Equal(StatusUnhealthy))
 
-			// Load a model so gRPC check is attempted
-			Expect(registry.SetNodeModel(context.Background(), node.ID, "test-model", "loaded", "10.0.0.5:50052")).To(Succeed())
-
-			// Create health monitor with a factory that returns healthy clients
-			factory := newFakeBackendClientFactory()
-			factory.setClient("10.0.0.5:50051", &fakeBackendClient{healthy: true})
-			hmWithFactory := NewHealthMonitor(registry, nil, 15*time.Second, 30*time.Second, "", false, factory)
+			// Refresh heartbeat (simulates the worker sending a heartbeat)
+			Expect(db.Model(&BackendNode{}).Where("id = ?", node.ID).
+				Update("last_heartbeat", time.Now()).Error).ToNot(HaveOccurred())
 
-			hmWithFactory.doCheckAll(context.Background())
+			hm.doCheckAll(context.Background())
 
 			fetched, err = registry.Get(context.Background(), node.ID)
 			Expect(err).ToNot(HaveOccurred())
@@ -207,44 +203,77 @@ var _ = Describe("HealthMonitor (mock-based)", func() {
 			Expect(calls).NotTo(ContainElement(ContainSubstring("MarkOffline")))
 		})
 
-		It("marks node unhealthy when gRPC check fails", func() {
+		It("keeps node healthy when heartbeat is fresh (with models loaded)", func() {
 			store := newFakeNodeHealthStore()
 			factory := newFakeBackendClientFactory()
 			hm := newTestHealthMonitor(store, factory, true, staleThreshold)
 
-			node := makeTestNode("node-5", "failing-worker", "10.0.0.5:50051", StatusHealthy, freshTime())
+			node := makeTestNode("node-5", "active-worker", "10.0.0.5:50051", StatusHealthy, freshTime())
 			store.addNode(node)
 			store.addNodeModel("node-5", NodeModel{NodeID: "node-5", ModelName: "llama-7b"})
 
-			// Configure gRPC health check to fail
-			factory.setClient("10.0.0.5:50051", &fakeBackendClient{
-				healthy: false,
-				err:     fmt.Errorf("connection refused"),
-			})
-
+			// No gRPC client needed — health is determined by heartbeat, not gRPC probe
 			hm.doCheckAll(context.Background())
 
-			Expect(store.getNode("node-5").Status).To(Equal(StatusUnhealthy))
-			Expect(store.getCalls()).To(ContainElement("MarkUnhealthy:node-5"))
+			Expect(store.getNode("node-5").Status).To(Equal(StatusHealthy))
+			calls := store.getCalls()
+			Expect(calls).NotTo(ContainElement(ContainSubstring("MarkUnhealthy")))
 		})
 
-		It("recovers unhealthy node when gRPC check succeeds", func() {
+		It("recovers unhealthy node when heartbeat is fresh", func() {
 			store := newFakeNodeHealthStore()
 			factory := newFakeBackendClientFactory()
 			hm := newTestHealthMonitor(store, factory, true, staleThreshold)
 
 			node := makeTestNode("node-6", "recovering-worker", "10.0.0.6:50051", StatusUnhealthy, freshTime())
 			store.addNode(node)
-			store.addNodeModel("node-6", NodeModel{NodeID: "node-6", ModelName: "llama-7b"})
-
-			// Configure gRPC health check to succeed
-			factory.setClient("10.0.0.6:50051", &fakeBackendClient{healthy: true})
 
 			hm.doCheckAll(context.Background())
 
-			// Should have called MarkHealthy to recover the node
 			Expect(store.getCalls()).To(ContainElement("MarkHealthy:node-6"))
 			Expect(store.getNode("node-6").Status).To(Equal(StatusHealthy))
 		})
+
+		It("node stays healthy when gRPC backend crashes but heartbeat is fresh", func() {
+			store := newFakeNodeHealthStore()
+			factory := newFakeBackendClientFactory()
+			hm := newTestHealthMonitor(store, factory, true, staleThreshold)
+
+			// Worker has a model loaded but the backend process crashed —
+			// node should remain healthy because heartbeat is fresh
+			node := makeTestNode("node-crash", "crash-worker", "10.0.0.9:50051", StatusHealthy, freshTime())
+			store.addNode(node)
+			store.addNodeModel("node-crash", NodeModel{NodeID: "node-crash", ModelName: "piper-model", Address: "10.0.0.9:50053"})
+
+			// gRPC backend is dead — but health is heartbeat-based, not gRPC-based
+			factory.setClient("10.0.0.9:50051", &fakeBackendClient{healthy: false, err: fmt.Errorf("connection refused")})
+
+			hm.doCheckAll(context.Background())
+
+			Expect(store.getNode("node-crash").Status).To(Equal(StatusHealthy))
+			calls := store.getCalls()
+			Expect(calls).NotTo(ContainElement(ContainSubstring("MarkUnhealthy")))
+		})
+
+		It("removes stale model via per-model health check without affecting node status", func() {
+			store := newFakeNodeHealthStore()
+			factory := newFakeBackendClientFactory()
+			hm := newTestHealthMonitor(store, factory, true, staleThreshold)
+			hm.perModelHealthCheck = true
+
+			node := makeTestNode("node-model", "model-worker", "10.0.0.10:50051", StatusHealthy, freshTime())
+			store.addNode(node)
+			store.addNodeModel("node-model", NodeModel{NodeID: "node-model", ModelName: "piper-model", Address: "10.0.0.10:50053"})
+
+			// Model backend is dead
+			factory.setClient("10.0.0.10:50053", &fakeBackendClient{healthy: false, err: fmt.Errorf("connection refused")})
+
+			hm.doCheckAll(context.Background())
+
+			// Node should remain healthy — only the model record is removed
+			Expect(store.getNode("node-model").Status).To(Equal(StatusHealthy))
+			Expect(store.getCalls()).To(ContainElement("RemoveNodeModel:node-model:piper-model"))
+			Expect(store.getCalls()).NotTo(ContainElement(ContainSubstring("MarkUnhealthy")))
+		})
 	})
 })