From 633de627011d8c06109685e8c06b55a0987382eb Mon Sep 17 00:00:00 2001 From: Flegma Date: Wed, 8 Jul 2026 15:12:06 +0200 Subject: [PATCH] fix(service-operation): honor max_retries in uptime service checks The services collection stores max_retries (UI "Retry Attempts") and the Go Service struct already deserializes it, but performCheck ran exactly one attempt and marked the service down on the first failure. The field was effectively write-only: notifications fired on a single slow or dropped probe. Retry the check up to max_retries times (2s apart, per-attempt timeout unchanged) and only mark the service down after every attempt fails. This mirrors the legacy frontend checker (httpChecker.ts used service.max_retries || 3). Unset or invalid values fall back to 3; values above 10 are clamped. Checks run in the per-service monitor goroutine, so retry sleeps never delay other services. Adds the module's first tests (monitoring/checker_test.go). --- .../service-operation/monitoring/checker.go | 170 ++++++++++++------ .../monitoring/checker_test.go | 130 ++++++++++++++ 2 files changed, 248 insertions(+), 52 deletions(-) create mode 100644 server/service-operation/monitoring/checker_test.go diff --git a/server/service-operation/monitoring/checker.go b/server/service-operation/monitoring/checker.go index a7c4a7a7..d1252484 100644 --- a/server/service-operation/monitoring/checker.go +++ b/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) } -} \ No newline at end of file +} + +// 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 +} diff --git a/server/service-operation/monitoring/checker_test.go b/server/service-operation/monitoring/checker_test.go new file mode 100644 index 00000000..9b2b832f --- /dev/null +++ b/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) + } +}