From d88d6066fcb45c92c8659cfa17bac9b08ad6dc1d Mon Sep 17 00:00:00 2001 From: Moses Narrow <36607567+0pcom@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:09:42 -0500 Subject: [PATCH] fix(visor): bound service-health probes so one dead endpoint can't stall `visor state` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ServiceHealth fans every /health probe out in parallel and wg.Wait()s on them. A single unresponsive endpoint — in practice a dmsg server whose /health does not answer over the discovery-routed path — blocked the whole call for the dmsg-HTTP client's full ~10s timeout. ServiceHealth is folded into the curated `visor state` snapshot, so that snapshot took ~10s and, under any additional load, tipped over callers' RPC timeouts and came back empty — which then reads downstream as "zero routes / no legs". Bound each probe with a per-request context timeout (4s). A dead endpoint now costs at most that, not the client timeout; healthy services (~300ms) are unaffected. Adds tests pinning both the bound and the healthy path. --- pkg/visor/api_services.go | 21 ++++++++- pkg/visor/api_services_probe_test.go | 65 ++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 pkg/visor/api_services_probe_test.go diff --git a/pkg/visor/api_services.go b/pkg/visor/api_services.go index 00bee4fead..96509d3745 100644 --- a/pkg/visor/api_services.go +++ b/pkg/visor/api_services.go @@ -122,13 +122,32 @@ func (v *Visor) ServiceHealth() ([]ServiceHealthEntry, error) { return results, nil } +// healthProbeTimeout bounds a single /health probe. ServiceHealth fans every +// probe out in parallel and wg.Wait()s on them, so an unresponsive endpoint +// (notably a dmsg server whose /health does not answer over the discovery-routed +// path) would otherwise stall the WHOLE call — and every caller of it — for the +// underlying dmsg-HTTP client's full timeout (~10s). That in turn made the +// curated `visor state` snapshot (which folds ServiceHealth in) take ~10s and +// time out under load, misreported downstream as empty/zero. A per-probe bound +// caps the blast radius: a dead endpoint costs at most this long, not the whole +// client timeout, while healthy services (normally ~300ms) are unaffected. +const healthProbeTimeout = 4 * time.Second + // doHealthProbe performs a single GET {baseURL}/health and populates a ServiceHealthEntry. func doHealthProbe(client *http.Client, name, baseURL, transport string) ServiceHealthEntry { //nolint:unparam entry := ServiceHealthEntry{Name: name, URL: baseURL, Transport: transport} reqURL := strings.TrimSuffix(baseURL, "/") + "/health" + ctx, cancel := context.WithTimeout(context.Background(), healthProbeTimeout) + defer cancel() start := time.Now() - resp, err := client.Get(reqURL) //nolint:gosec + req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil) + if err != nil { + entry.Status = "DOWN" + entry.Error = err.Error() + return entry + } + resp, err := client.Do(req) //nolint:gosec entry.LatencyMs = time.Since(start).Milliseconds() if err != nil { diff --git a/pkg/visor/api_services_probe_test.go b/pkg/visor/api_services_probe_test.go new file mode 100644 index 0000000000..69a0d9e2ef --- /dev/null +++ b/pkg/visor/api_services_probe_test.go @@ -0,0 +1,65 @@ +// Package visor pkg/visor/api_services_probe_test.go c3-vis-core +package visor + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// TestDoHealthProbe_BoundsHangingEndpoint pins the fix for the ~10s ServiceHealth +// stall: a single unresponsive /health endpoint must NOT block the probe for the +// underlying client's full timeout. doHealthProbe caps each request at +// healthProbeTimeout, so a dead endpoint returns DOWN promptly instead of +// stalling ServiceHealth (and the `visor state` snapshot that folds it in). +func TestDoHealthProbe_BoundsHangingEndpoint(t *testing.T) { + // A server that never responds within the probe budget: it blocks until the + // request context is canceled — which is exactly what doHealthProbe's timeout + // does to the underlying connection. That both exercises the bound and lets + // httptest tear down promptly once the probe gives up. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-r.Context().Done() + })) + defer srv.Close() + + start := time.Now() + entry := doHealthProbe(srv.Client(), "Hanging", srv.URL, "http") + elapsed := time.Since(start) + + // Must return well before the server would have (10s past the budget), + // bounded by healthProbeTimeout plus a little scheduling slack. + if elapsed > healthProbeTimeout+2*time.Second { + t.Fatalf("probe took %v; expected it bounded near healthProbeTimeout (%v)", elapsed, healthProbeTimeout) + } + if entry.Status != "DOWN" { + t.Fatalf("hanging endpoint status = %q; want DOWN", entry.Status) + } +} + +// TestDoHealthProbe_HealthyIsUnaffected confirms the bound does not penalize a +// normally-responding service: it returns OK quickly with the version parsed. +func TestDoHealthProbe_HealthyIsUnaffected(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte(`{"build_info":{"version":"v1.2.3"}}`)); err != nil { + t.Errorf("write health body: %v", err) + } + })) + defer srv.Close() + + start := time.Now() + entry := doHealthProbe(srv.Client(), "Healthy", srv.URL, "http") + elapsed := time.Since(start) + + if elapsed > healthProbeTimeout { + t.Fatalf("healthy probe took %v; should be near-instant", elapsed) + } + if entry.Status != "OK" { + t.Fatalf("healthy status = %q; want OK", entry.Status) + } + if entry.Version != "v1.2.3" { + t.Fatalf("version = %q; want v1.2.3", entry.Version) + } +}