From aad01b8d6fcd61cce9fcdc514447ffa776e7c7f3 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 3 Sep 2026 02:47:51 +0100 Subject: [PATCH] fix(target-health): keep sweep timestamp first for atomic alignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit targetHealthLastSweepUnixNano moves to Gateway's first field. It is read and written only through sync/atomic's 64-bit functions, and the sync/atomic package guarantees 64-bit alignment on ARM, x86-32, and 32-bit MIPS only for a struct's first word — Traefik ships armv7 and 386 builds alongside amd64/arm64. Every other field keeps golangci-lint fieldalignment's pointer-grouped ordering; verified with the standalone fieldalignment analyzer that this is the only field out of place and adds no struct-size padding, only a documented +8 GC-scan-prefix cost. docs: catches up README's admin `health` object and Target health "State" bullet to F4 (previous commit, a0c8607) — three ways to read `state: "unknown"`, and that consecutiveFailures/lastCheck/ lastError/latencyMs/source stay present when a target has been observed but never succeeded. Claude-Session: https://claude.ai/code/session_01EebV5jyWvYocByNQ3oss1w --- README.md | 34 ++++++++++++++++----- llmgateway.go | 83 ++++++++++++++++++++++++++++++++------------------- 2 files changed, 78 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 6a094d0..49b826b 100644 --- a/README.md +++ b/README.md @@ -1549,9 +1549,11 @@ or in CI. `health` is `{"state","lastCheck","lastError","consecutiveFailures","latencyMs","source"}` — see [MCP and A2A](#mcp-and-a2a)'s "Target health" section for the full contract. `state` is `"unknown"`, `"healthy"`, or `"unhealthy"`, - and `consecutiveFailures` is always present; every other field is - omitted while `state` is `"unknown"` (this target has never been - observed). This route always reads existing state; when + and `consecutiveFailures` is always present. Every other field is + omitted only while this target has never once been observed; `state` + alone does not decide the omission, since `"unknown"` also covers a + target that has been observed but has not yet succeeded, and that case + keeps every field. This route always reads existing state; when `targetHealth.enabled`, it additionally triggers a background probe sweep when one is due, so the response itself never waits on that sweep. Polled every 5 seconds, in the same batch as `overview` and @@ -1921,11 +1923,27 @@ plugin's config. or the default `/.well-known/agent-card.json`); a `404` still counts as reachable — for example an umbrella agent whose root carries no card of its own. - - **State**: `"unknown"` (never observed), `"healthy"` (recorded at - least once, `consecutiveFailures` below `targetHealth.failureThreshold`), - or `"unhealthy"` (`consecutiveFailures` at or above the threshold). A - target with one or two recent failures after a success still reads - `"healthy"` until the threshold is actually crossed. + - **State**: `"healthy"` (succeeded at least once, + `consecutiveFailures` below `targetHealth.failureThreshold`), or + `"unhealthy"` (`consecutiveFailures` at or above the threshold, + whether or not the target has ever succeeded). A target with one or + two recent failures after a success still reads `"healthy"` until the + threshold is actually crossed. + - **`"unknown"` reads three ways**, all sharing one JSON `state` + value: + 1. never observed at all — no probe or passive traffic has reached + this target yet. `GET /admin/api/targets` omits every health field + but `state`/`consecutiveFailures` for this case. + 2. observed, but never once succeeded, and still below + `targetHealth.failureThreshold` — the threshold's protection + against a single transient first failure still applies, so this + does not jump straight to `"unhealthy"`, but a target this tracker + has never seen answer is not `"healthy"` either. Every other + health field is still present here, since there is a real + observation to show. + 3. either way, `llmgateway_target_healthy` emits no sample while + `state` is `"unknown"` — a gap in the series is honest, a + fabricated `1` is not. ## Security notes diff --git a/llmgateway.go b/llmgateway.go index 116678b..eceff6f 100644 --- a/llmgateway.go +++ b/llmgateway.go @@ -586,7 +586,26 @@ const maxExplicitBodyAdmissionCap = 10_000 // convention (admin.go) for the same reasoning spelled out once; this // struct is the other place it applies. type Gateway struct { - next http.Handler + // targetHealthLastSweepUnixNano MUST stay the first field (F7, + // feat/target-health review). It backs maybeSweepTargetHealth's + // single-flight gate (target_health.go) and is read/written only via + // sync/atomic's 64-bit functions (atomic.LoadInt64/StoreInt64) — the + // sync/atomic package documents that "on ARM, x86-32, and 32-bit + // MIPS, it is the caller's responsibility to arrange for 64-bit + // alignment of 64-bit words accessed atomically," and guarantees + // that alignment only for "the first word in an allocated struct, + // array, or slice." Traefik ships armv7 and 386 builds alongside + // amd64/arm64, so this is not a hypothetical: any field placed + // ahead of this one could silently misalign it on those platforms. + // Every field below is ordered by golangci-lint's fieldalignment + // linter (pointer-containing fields grouped first, for GC scan + // efficiency, then the rest) — this one field is the sole, deliberate + // exception, verified with `fieldalignment ./...` to add no + // additional padding: an 8-byte int64 transitions cleanly into the + // 8-byte-aligned pointer block that follows on every architecture + // this plugin targets. + targetHealthLastSweepUnixNano int64 + next http.Handler // redisClient is the same instance newGateway hands to both the // limiter's redisStore and the response cache (its own doc comment, // below, explains why it's built once and shared) — kept here too, @@ -617,19 +636,22 @@ type Gateway struct { adapters map[string]providerAdapter cfg *Config auth *authStore + // provenance is the in-process usage-accounting-provenance accumulator + // (feat: expose token-accounting provenance, metrics.go's + // provenanceStore) — recordUsageProvenance's only write target, and + // writeProvenanceMetrics/buildAdminProvenanceViews' (admin.go) own + // read source. Always constructed, here, by newGateway, even when + // Config.Metrics is nil or disabled, mirroring g.latency's own + // identical "always-present, usually-empty, nil-receiver-safe" + // convention immediately below — see that field's own doc comment for + // the full reasoning, which applies here unchanged. + provenance *provenanceStore // cache is nil whenever response caching is not configured or not // usable (cfg.Cache.Enabled is false, or true with no config.Redis — // see buildResponseCache, cache.go). Every call site checks for nil // before using it, rather than responseCache having its own // always-disabled zero value. cache *responseCache - // bodyAdmission is the buffered-channel semaphore acquireBodyAdmission - // (routes_unified.go) claims from and releases: security review - // finding 1b, 2026-08-22. Sized once, here, by newGateway (see - // defaultBodyAdmissionCap/Config.MaxInFlightBodyRequests) — never - // resized afterward, matching a Go channel's own fixed-capacity - // contract. - bodyAdmission chan struct{} // targetClient is the shared, connection-pooled *http.Client the // MCP/A2A target proxy (mcp_a2a.go) issues every upstream request // through — built once via newAdapterHTTPClient, the same constructor @@ -648,17 +670,14 @@ type Gateway struct { // identical "safe even off a bare &Gateway{} literal" convention // above. latency *latencyStore - // provenance is the in-process usage-accounting-provenance accumulator - // (feat: expose token-accounting provenance, metrics.go's - // provenanceStore) — recordUsageProvenance's only write target, and - // writeProvenanceMetrics/buildAdminProvenanceViews' (admin.go) own - // read source. Always constructed, here, by newGateway, even when - // Config.Metrics is nil or disabled, mirroring g.latency's own - // identical "always-present, usually-empty, nil-receiver-safe" - // convention immediately above — see that field's own doc comment for - // the full reasoning, which applies here unchanged. - provenance *provenanceStore - name string + // bodyAdmission is the buffered-channel semaphore acquireBodyAdmission + // (routes_unified.go) claims from and releases: security review + // finding 1b, 2026-08-22. Sized once, here, by newGateway (see + // defaultBodyAdmissionCap/Config.MaxInFlightBodyRequests) — never + // resized afterward, matching a Go channel's own fixed-capacity + // contract. + bodyAdmission chan struct{} + name string // failoverLogGate rate-limits runMeteredCall's generic "failing over" // log line (routes_unified.go) to once per storeErrorLogEvery // (adversarial-review fix, F10) — reuses auth.go's own logGate type, @@ -672,13 +691,13 @@ type Gateway struct { // never re-parses a CIDR string per scrape. nil when Metrics is // unconfigured or carries no AllowedCIDRs entries. metricsNets []*net.IPNet - // failover is Config.Failover, validated and resolved once by - // newGateway (validateFailoverConfig, failover.go). - failover failoverConfig // targetHealthCfg is Config.TargetHealth, validated and resolved once // by validateTargetHealthConfig (target_health.go) — maybeSweepTargetHealth // reads probeInterval/enabled from this, never the raw Config. targetHealthCfg targetHealthConfig + // failover is Config.Failover, validated and resolved once by + // newGateway (validateFailoverConfig, failover.go). + failover failoverConfig // targetTimeout is targetClient's own resolved request timeout — // Config.RequestTimeout only; an MCP/A2A target has no per-target // override the way a provider does. proxyUpstream's own callers @@ -686,15 +705,17 @@ type Gateway struct { // idle-progress body watchdog (timeout.go) matches the // ResponseHeaderTimeout already set on targetClient's Transport. targetTimeout time.Duration - // targetHealthLastSweepUnixNano/targetHealthSweeping back - // maybeSweepTargetHealth's own single-flight, at-most-one-sweep-per- - // interval gate (target_health.go): a UnixNano timestamp and a CAS - // flag, both atomic rather than mutex-guarded, since every caller - // (serveMetrics, serveAdminTargets, handleMCPServers, handleAgents) - // reaches maybeSweepTargetHealth on a live request path and must - // never block behind a lock another such call already holds. - targetHealthLastSweepUnixNano int64 - targetHealthSweeping int32 + // targetHealthSweeping is maybeSweepTargetHealth's own single-flight + // CAS flag (target_health.go) — its sibling, the UnixNano timestamp + // this same gate reads, is targetHealthLastSweepUnixNano, kept as + // this struct's FIRST field instead (see that field's own doc + // comment, top of this struct, for why). Both are atomic rather than + // mutex-guarded, since every caller (serveMetrics, serveAdminTargets, + // handleMCPServers, handleAgents) reaches maybeSweepTargetHealth on a + // live request path and must never block behind a lock another such + // call already holds. A plain int32 needs no special alignment + // treatment the way its int64 sibling does. + targetHealthSweeping int32 } // telemetryStartupOnce keeps the anonymous "plugin loaded" ping to one per