fix(service-operation): honor max_retries in uptime service checks

server/service-operation/monitoring/checker.go

@@ -2,6 +2,7 @@
 package monitoring
 
 import (
+	"errors"
 	"log"
 	"strings"
 	"time"
@@ -12,6 +13,25 @@ import (
 	"service-operation/types"
 )
 
+const (
+	// defaultCheckTimeout is the per-attempt timeout for a service check.
+	defaultCheckTimeout = 10 * time.Second
+
+	// defaultMaxRetries matches the UI default ("3 attempts") and the
+	// legacy frontend checker behavior (service.max_retries || 3).
+	defaultMaxRetries = 3
+
+	// maxRetriesCap bounds user-provided values so one check cycle can
+	// never block a service monitor loop for an excessive time.
+	maxRetriesCap = 10
+
+	// retryDelay is the pause between consecutive attempts.
+	retryDelay = 2 * time.Second
+)
+
+// errUnsupportedServiceType marks service types the checker cannot handle.
+var errUnsupportedServiceType = errors.New("unsupported service type")
+
 func (ms *MonitoringService) performCheck(service pocketbase.Service) {
 	// First, fetch the latest service status from PocketBase to ensure we have current data
 	latestService, err := ms.pbClient.GetService(service.ID)
@@ -25,63 +45,22 @@ func (ms *MonitoringService) performCheck(service pocketbase.Service) {
 		return // Silently skip paused services
 	}
 
-	timeout := 10 * time.Second // Default timeout
-	var result *types.OperationResult
-
-	serviceType := strings.ToLower(latestService.ServiceType)
-
-	// Single log message for check start
-	//log.Printf("Checking %s (%s)", latestService.Name, serviceType)
-
-	switch serviceType {
-	case "ping", "icmp":
-		pingOp := operations.NewPingOperation(timeout)
-		host := latestService.Host
-		if host == "" {
-			host = latestService.URL
-		}
-		result, err = pingOp.Execute(host, 1) // Single ping for monitoring
-
-	case "dns":
-		dnsOp := operations.NewDNSOperation(timeout)
-		host := latestService.Host
-		if host == "" {
-			host = latestService.Domain
-		}
-		// Default to A record, but could be made configurable
-		queryType := "A"
-		result, err = dnsOp.Execute(host, queryType)
-
-	case "tcp":
-		tcpOp := operations.NewTCPOperation(timeout)
-		host := latestService.Host
-		if host == "" {
-			host = latestService.URL
-		}
-		port := latestService.Port
-		if port <= 0 {
-			port = 80 // Default port
-		}
-		result, err = tcpOp.Execute(host, port)
-
-	case "http", "https":
-		httpOp := operations.NewHTTPOperation(timeout)
-		url := latestService.URL
-		if url == "" {
-			url = latestService.Host
-		}
-		result, err = httpOp.Execute(url, "GET")
-
-	default:
+	// Honor the per-service retry configuration (services.max_retries).
+	// The service is only marked down after ALL attempts fail, so one
+	// slow or dropped probe no longer flaps the service to "down".
+	maxRetries := normalizeMaxRetries(latestService.MaxRetries)
+
+	result, err := runCheckAttempts(latestService, maxRetries, defaultCheckTimeout, retryDelay)
+	if errors.Is(err, errUnsupportedServiceType) {
 		log.Printf("Unknown service type: %s for service %s", latestService.ServiceType, latestService.Name)
 		return
 	}
 
-	// Determine status based on result
+	// Determine status based on the final attempt
 	status := "down"
 	errorMessage := ""
 	responseTime := int64(0)
-	
+
 	if err != nil {
 		errorMessage = err.Error()
 		log.Printf("❌ %s failed: %v", latestService.Name, err)
@@ -104,7 +83,7 @@ func (ms *MonitoringService) performCheck(service pocketbase.Service) {
 		log.Printf("Failed to verify service status before update for %s: %v", latestService.Name, err)
 		return
 	}
-	
+
 	if currentService.Status == "paused" {
 		return // Silently skip status update for paused services
 	}
@@ -120,4 +99,91 @@ func (ms *MonitoringService) performCheck(service pocketbase.Service) {
 		metricsSaver := savers.NewMetricsSaverWithRegion(ms.pbClient, regionName, agentID)
 		metricsSaver.SaveMetricsForService(*latestService, result)
 	}
-}
+}
+
+// normalizeMaxRetries clamps the configured max_retries to a sane range,
+// falling back to the default when the field is unset (0) or invalid.
+func normalizeMaxRetries(configured int) int {
+	if configured <= 0 {
+		return defaultMaxRetries
+	}
+	if configured > maxRetriesCap {
+		return maxRetriesCap
+	}
+	return configured
+}
+
+// runCheckAttempts executes up to maxRetries check attempts for the service,
+// waiting delay between attempts. It returns as soon as one attempt succeeds;
+// otherwise it returns the outcome of the final attempt.
+func runCheckAttempts(service *pocketbase.Service, maxRetries int, timeout, delay time.Duration) (*types.OperationResult, error) {
+	var result *types.OperationResult
+	var err error
+
+	for attempt := 1; attempt <= maxRetries; attempt++ {
+		result, err = executeCheck(service, timeout)
+		if errors.Is(err, errUnsupportedServiceType) {
+			return nil, err
+		}
+
+		if err == nil && result != nil && result.Success {
+			if attempt > 1 {
+				log.Printf("✅ %s succeeded on attempt %d/%d", service.Name, attempt, maxRetries)
+			}
+			return result, nil
+		}
+
+		if attempt < maxRetries {
+			log.Printf("⚠️ %s check failed (attempt %d/%d), retrying in %s", service.Name, attempt, maxRetries, delay)
+			time.Sleep(delay)
+		}
+	}
+
+	return result, err
+}
+
+// executeCheck runs a single check attempt for the service. It is a plain
+// function (it needs no MonitoringService state) so tests can call it directly.
+func executeCheck(service *pocketbase.Service, timeout time.Duration) (*types.OperationResult, error) {
+	switch strings.ToLower(service.ServiceType) {
+	case "ping", "icmp":
+		pingOp := operations.NewPingOperation(timeout)
+		host := service.Host
+		if host == "" {
+			host = service.URL
+		}
+		return pingOp.Execute(host, 1) // Single ping for monitoring
+
+	case "dns":
+		dnsOp := operations.NewDNSOperation(timeout)
+		host := service.Host
+		if host == "" {
+			host = service.Domain
+		}
+		// Default to A record, but could be made configurable
+		queryType := "A"
+		return dnsOp.Execute(host, queryType)
+
+	case "tcp":
+		tcpOp := operations.NewTCPOperation(timeout)
+		host := service.Host
+		if host == "" {
+			host = service.URL
+		}
+		port := service.Port
+		if port <= 0 {
+			port = 80 // Default port
+		}
+		return tcpOp.Execute(host, port)
+
+	case "http", "https":
+		httpOp := operations.NewHTTPOperation(timeout)
+		url := service.URL
+		if url == "" {
+			url = service.Host
+		}
+		return httpOp.Execute(url, "GET")
+	}
+
+	return nil, errUnsupportedServiceType
+}

server/service-operation/monitoring/checker_test.go

@@ -0,0 +1,130 @@
+package monitoring
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"service-operation/pocketbase"
+)
+
+func TestNormalizeMaxRetries(t *testing.T) {
+	tests := []struct {
+		name       string
+		configured int
+		want       int
+	}{
+		{"unset falls back to default", 0, defaultMaxRetries},
+		{"negative falls back to default", -2, defaultMaxRetries},
+		{"single attempt kept as-is", 1, 1},
+		{"ui maximum kept as-is", 5, 5},
+		{"excessive value clamped to cap", 100, maxRetriesCap},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := normalizeMaxRetries(tt.configured); got != tt.want {
+				t.Errorf("normalizeMaxRetries(%d) = %d, want %d", tt.configured, got, tt.want)
+			}
+		})
+	}
+}
+
+// flakyServer returns a test server that fails with HTTP 500 for the first
+// failures requests and responds 200 afterwards, plus a hit counter.
+func flakyServer(failures int64) (*httptest.Server, *int64) {
+	var hits int64
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		n := atomic.AddInt64(&hits, 1)
+		if n <= failures {
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+		w.WriteHeader(http.StatusOK)
+	}))
+	return server, &hits
+}
+
+func TestRunCheckAttemptsRecoversWithinRetryBudget(t *testing.T) {
+	server, hits := flakyServer(2)
+	defer server.Close()
+
+	service := &pocketbase.Service{
+		Name:        "flaky",
+		ServiceType: "http",
+		URL:         server.URL,
+	}
+
+	result, err := runCheckAttempts(service, 3, 5*time.Second, 10*time.Millisecond)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if result == nil || !result.Success {
+		t.Fatalf("expected success after retries, got result=%+v", result)
+	}
+	if got := atomic.LoadInt64(hits); got != 3 {
+		t.Errorf("expected 3 attempts, server saw %d", got)
+	}
+}
+
+func TestRunCheckAttemptsSingleAttemptStaysSingleShot(t *testing.T) {
+	server, hits := flakyServer(1)
+	defer server.Close()
+
+	service := &pocketbase.Service{
+		Name:        "single-shot",
+		ServiceType: "http",
+		URL:         server.URL,
+	}
+
+	result, err := runCheckAttempts(service, 1, 5*time.Second, 10*time.Millisecond)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if result == nil || result.Success {
+		t.Fatalf("expected failure with a single attempt, got result=%+v", result)
+	}
+	if got := atomic.LoadInt64(hits); got != 1 {
+		t.Errorf("expected exactly 1 attempt, server saw %d", got)
+	}
+}
+
+func TestRunCheckAttemptsMarksDownAfterAllAttemptsFail(t *testing.T) {
+	server, hits := flakyServer(1000)
+	defer server.Close()
+
+	service := &pocketbase.Service{
+		Name:        "always-down",
+		ServiceType: "http",
+		URL:         server.URL,
+	}
+
+	result, err := runCheckAttempts(service, 3, 5*time.Second, 10*time.Millisecond)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if result == nil || result.Success {
+		t.Fatalf("expected failure after exhausting retries, got result=%+v", result)
+	}
+	if got := atomic.LoadInt64(hits); got != 3 {
+		t.Errorf("expected exactly 3 attempts, server saw %d", got)
+	}
+}
+
+func TestExecuteCheckUnsupportedServiceType(t *testing.T) {
+	service := &pocketbase.Service{
+		Name:        "mystery",
+		ServiceType: "carrier-pigeon",
+	}
+
+	if _, err := executeCheck(service, time.Second); !errors.Is(err, errUnsupportedServiceType) {
+		t.Fatalf("expected errUnsupportedServiceType, got %v", err)
+	}
+
+	if _, err := runCheckAttempts(service, 3, time.Second, time.Millisecond); !errors.Is(err, errUnsupportedServiceType) {
+		t.Fatalf("expected errUnsupportedServiceType from runCheckAttempts, got %v", err)
+	}
+}