From dada5a316976883633d95b763697c5b631d0b037 Mon Sep 17 00:00:00 2001 From: zeekay Date: Sat, 25 Jul 2026 15:10:46 -0700 Subject: [PATCH] health,build: make /v1/health and /v1/metrics report reality MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two endpoints were answering with the right status code and an empty truth, so every fleet looked instrumented while reporting nothing. GET /v1/health encoded apihealth.APIReply through jsonv2, which has no default representation for time.Duration — and every Result carries one. The marshal therefore failed on every node, every time, and the handler fell back to {"healthy":…,"error":"health reply encode failed"}. The status code is written before the encode, so k8s probes and dashboards stayed green while the body carried no checks at all. Measured on Zoo, Hanzo and Pars mainnet, node v1.34.9 and v1.36.2 alike. The type is defined with encoding/json tags and the POST (jsonrpc) path already encodes it through that codec, which is why POST returned the full check set while GET returned nothing. One wire type deserves one encoder: GET now uses encoding/json too, so the two paths agree by construction. encoding/json also maps invalid UTF-8 in check Details to U+FFFD instead of failing, which is the behaviour the old jsontext AllowInvalidUTF8 option was reaching for. The buffer stays: it keeps a partial encode from shipping a torn body. /v1/metrics answered 200 with zero bytes for the same shape of reason. luxfi/metric resolves NewRegistry() to a no-op registry unless the binary is built with -tags metrics (registry_noop.go, //go:build !metrics), and only the `full` profile passed it. Images build with the default `minimal` profile, so every metric in luxd registered into a black hole while --api-metrics-enabled=true still advertised metrics as on. Whether metrics are served is the runtime flag's call; a build tag must not silently overrule it. `metrics` becomes a base tag on every profile. Verified: the three new handler tests fail against the jsonv2 encoder (checks decode empty, error field nil — the exact production symptom) and pass after. `go tool nm` shows (*registry).Gather absent from a default build and present once the tag is set. Co-authored-by: Hanzo Dev --- scripts/build.sh | 18 ++++-- service/health/handler.go | 26 +++++---- service/health/handler_test.go | 103 +++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 15 deletions(-) create mode 100644 service/health/handler_test.go diff --git a/scripts/build.sh b/scripts/build.sh index e0af4a992..04c096d07 100755 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -54,8 +54,16 @@ source "${REPO_ROOT}"/scripts/constants.sh # Determine the git commit hash to use for the build source "${REPO_ROOT}"/scripts/git_commit.sh -# Configure build based on profile -tags="" +# Configure build based on profile. +# +# `metrics` is a base tag, not a profile choice: without it luxfi/metric's +# NewRegistry() resolves to the no-op registry (registry_noop.go, //go:build +# !metrics), every metric registers into a black hole, and /v1/metrics answers +# 200 with a zero-byte body — while --api-metrics-enabled=true still reports +# metrics as on. Whether metrics are served is the runtime flag's decision +# alone; the build must not silently overrule it. +base_tags="metrics" +tags="${base_tags}" ldflags="-X github.com/luxfi/node/version.GitCommit=$git_commit \ -X github.com/luxfi/node/version.VersionMajor=$version_major \ -X github.com/luxfi/node/version.VersionMinor=$version_minor \ @@ -67,7 +75,7 @@ upx_compress=false case "${profile}" in minimal) echo "Profile: minimal (ZAP, all VMs, NAT, stripped)" - tags="nattraversal" + tags="${base_tags},nattraversal" strip_flags="-s -w" ;; core) @@ -77,7 +85,7 @@ case "${profile}" in ;; full) echo "Profile: full (gRPC+ZAP, all VMs, all features, stripped)" - tags="grpc,nattraversal,zxcvbn,metrics" + tags="${base_tags},grpc,nattraversal,zxcvbn" strip_flags="-s -w" ;; dev) @@ -86,7 +94,7 @@ case "${profile}" in ;; tiny) echo "Profile: tiny (all VMs, NAT + UPX compressed)" - tags="nattraversal" + tags="${base_tags},nattraversal" strip_flags="-s -w" upx_compress=true ;; diff --git a/service/health/handler.go b/service/health/handler.go index 85302acdb..d22c88f1b 100644 --- a/service/health/handler.go +++ b/service/health/handler.go @@ -5,11 +5,10 @@ package health import ( "bytes" + "encoding/json" "fmt" "net/http" - "github.com/go-json-experiment/json" - "github.com/go-json-experiment/json/jsontext" "github.com/gorilla/rpc/v2" apihealth "github.com/luxfi/api/health" @@ -56,17 +55,24 @@ func NewGetHandler(reporter func(tags ...string) (map[string]apihealth.Result, b // If a health check has failed, we should return a 503. w.WriteHeader(http.StatusServiceUnavailable) } - // Buffer the reply — a streaming encoder that errors part-way - // (jsonv2 rejects invalid UTF-8, and check Details may embed raw - // chain-ID bytes) leaves the client a torn body whose - // Content-Length matches the truncation. Buffering makes the - // reply atomic; AllowInvalidUTF8 turns binary detail bytes into - // replacement runes instead of an encode error. + // One encoder for one wire type: apihealth.APIReply is defined with + // encoding/json tags and carries a time.Duration per check, so it is + // encoding/json that defines its representation. The POST (jsonrpc) + // path already encodes it through that codec; encoding it here the + // same way is what makes GET and POST agree. jsonv2 cannot encode + // this type at all — time.Duration has no default representation + // there — so every GET reply used to degrade to the error fallback + // below. encoding/json also replaces invalid UTF-8 (check Details + // may embed raw chain-ID bytes) with U+FFFD rather than failing. + // + // Buffer first: a streaming encoder that errors part-way would leave + // the client a torn body whose Content-Length matches the + // truncation. Buffering makes the reply atomic. var buf bytes.Buffer - err := json.MarshalWrite(&buf, apihealth.APIReply{ + err := json.NewEncoder(&buf).Encode(apihealth.APIReply{ Checks: checks, Healthy: healthy, - }, jsontext.AllowInvalidUTF8(true)) + }) if err != nil { buf.Reset() fmt.Fprintf(&buf, `{"healthy":%t,"error":"health reply encode failed"}`, healthy) diff --git a/service/health/handler_test.go b/service/health/handler_test.go new file mode 100644 index 000000000..e234eb9df --- /dev/null +++ b/service/health/handler_test.go @@ -0,0 +1,103 @@ +// Copyright (C) 2019-2025, Lux Industries Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package health + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/require" + + apihealth "github.com/luxfi/api/health" +) + +// The GET handler must emit the real check set. It previously encoded through +// jsonv2, which cannot represent apihealth.Result.Duration (a time.Duration), +// so every reply on every node degraded to {"healthy":…,"error":"health reply +// encode failed"} while the status code still looked correct — k8s probes +// passed and operators saw nothing. +func TestGetHandlerEncodesDurationBearingChecks(t *testing.T) { + require := require.New(t) + + checks := map[string]apihealth.Result{ + "bls": { + Details: "node has the correct BLS key", + Duration: 134597 * time.Nanosecond, + Timestamp: time.Unix(1753479996, 0).UTC(), + }, + } + h := NewGetHandler(func(...string) (map[string]apihealth.Result, bool) { + return checks, true + }) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1/health", nil)) + + require.Equal(http.StatusOK, rec.Code) + + var reply apihealth.APIReply + require.NoError(json.Unmarshal(rec.Body.Bytes(), &reply)) + require.True(reply.Healthy) + require.Len(reply.Checks, 1) + require.Equal(134597*time.Nanosecond, reply.Checks["bls"].Duration) + require.NotContains(rec.Body.String(), "health reply encode failed") +} + +// An unhealthy node must still return the full diagnostic body alongside the +// 503 — the body is the only thing that says *why*. +func TestGetHandlerUnhealthyStillCarriesChecks(t *testing.T) { + require := require.New(t) + + errMsg := "network layer is unhealthy reason: primary network validator has no inbound connections" + checks := map[string]apihealth.Result{ + "network": { + Details: map[string]any{"connectedPeers": 4}, + Error: &errMsg, + Duration: 40357 * time.Nanosecond, + ContiguousFailures: 31997, + }, + } + h := NewGetHandler(func(...string) (map[string]apihealth.Result, bool) { + return checks, false + }) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1/health", nil)) + + require.Equal(http.StatusServiceUnavailable, rec.Code) + + var reply apihealth.APIReply + require.NoError(json.Unmarshal(rec.Body.Bytes(), &reply)) + require.False(reply.Healthy) + require.Equal(&errMsg, reply.Checks["network"].Error) + require.Equal(int64(31997), reply.Checks["network"].ContiguousFailures) +} + +// Check Details may embed raw chain-ID bytes. Invalid UTF-8 must degrade to +// replacement runes, never to an encode failure that drops the whole reply. +func TestGetHandlerInvalidUTF8InDetailsDoesNotDropReply(t *testing.T) { + require := require.New(t) + + checks := map[string]apihealth.Result{ + "database": { + Details: string([]byte{0xff, 0xfe, 0x00}), + Duration: 19137 * time.Nanosecond, + }, + } + h := NewGetHandler(func(...string) (map[string]apihealth.Result, bool) { + return checks, true + }) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/v1/health", nil)) + + require.Equal(http.StatusOK, rec.Code) + var reply apihealth.APIReply + require.NoError(json.Unmarshal(rec.Body.Bytes(), &reply)) + require.Len(reply.Checks, 1) + require.NotContains(rec.Body.String(), "health reply encode failed") +}