diff --git a/README.md b/README.md index 3146250..d98ee47 100644 --- a/README.md +++ b/README.md @@ -1406,9 +1406,26 @@ plugin's config. - `llmgateway_provider_healthy{provider}` — `1`/`0` gauge from the discovery circuit breaker (`healthState` in the admin API); see [Provider health](#provider-health-discovery-circuit-breaker). + - `llmgateway_limit_store_up` — `1`/`0` gauge for this replica's own + connection to the configured limit store (Redis); absent entirely + when no store is configured. Every store-backed family above simply + goes silent, scope by scope, for whatever it cannot currently read — + the honest choice for those families, but on its own that gives a + store outage no independent signal on this endpoint (with + `redis.failOpen: false` and a store flapping faster than its 5s fail + latch, every store-backed family can be absent scrape after scrape + with nothing else here saying why). This gauge is that signal. - `llmgateway_rate_limit_rejections_total{scope_kind,scope_id}` — how many requests a rate or budget limit actually refused, by scope. This one has no equivalent in the admin JSON API: nothing else tracks it. + It does **not** reset only on process restart: a scope's own count + also resets to `0` when that user is removed via a hot-reloaded users + file, and **every** tracked scope resets together, in one bulk cliff, + the instant more than 2000 distinct scopes have ever been seen in + this process's lifetime (a bounded-memory safeguard, not a graceful + per-scope decay). A value that unexpectedly drops is one of these + events, not a scrape anomaly — see the live `# HELP` line for the + exact current threshold. - **Aggregation across replicas — read this before wiring an alert**: with `redis` configured, **every replica reads the SAME shared counters** @@ -1425,7 +1442,7 @@ plugin's config. identical families become genuinely **per-replica** instead — each replica counts only the traffic it personally handled — and `sum()` becomes the correct aggregation instead. This flips on that one config - bit; know which one you are running before wiring an alert. Two + bit; know which one you are running before wiring an alert. Three families do **not** follow this rule: - `llmgateway_rate_limit_rejections_total` lives only in each replica's own process memory, **never** in Redis, regardless of the `redis` @@ -1437,24 +1454,41 @@ plugin's config. possible; if you must aggregate, `min()` surfaces "at least one replica sees this provider as unhealthy" — `sum()` is meaningless for a 0/1 gauge. + - `llmgateway_limit_store_up` reflects each replica's own, locally + -discovered connection health to the configured store — it cannot + itself be read from the store it describes, so it is inherently + per-replica too. Same guidance as `llmgateway_provider_healthy`: + prefer per-instance, `min()` if you must aggregate. - **Labels are escaped**: a label value that is user- or upstream-controlled (a user/group name, a model id) is escaped for - backslash, double quote, and newline, byte-for-byte — the exact rule - the text-exposition format requires. This matters more than it looks: - Prometheus reports an unparseable scrape as the **whole target being - down**, not as one bad line, so a single unescaped quote in a model id - would otherwise take the entire endpoint offline from Prometheus's point - of view, not just that one series. + backslash, double quote, and newline — the exact rule the + text-exposition format requires. This matters more than it looks: an + unescaped quote or newline breaks the exposition format's own lexical + structure (an unterminated quoted string), and Prometheus reports an + unparseable scrape as the **whole target being down**, not as one bad + line — so a single unescaped quote in a model id would otherwise take + the entire endpoint offline from Prometheus's point of view, not just + that one series. A raw, invalid UTF-8 byte is escaped too, as a + deterministic `\xHH` marker rather than passed through unchanged: an + invalid-UTF-8 label value is independently fatal to a real Prometheus + scrape (measured against `promparse.go`), so the escaping keeps every + distinct input distinguishable (no two different names collapse onto + one series) while guaranteeing the output stays valid UTF-8 either way. - **A store outage never fabricates a zero**: if the configured Redis is - unreachable and `redis.failOpen` is `false`, an affected scope's - request/token/cost/provider counters are **skipped entirely** for that - scrape rather than reported as `0` — a temporary gap in the series is - the honest signal; a fabricated `0` would read to Prometheus as a - counter reset followed by a phantom spike the instant the store - recovers. `llmgateway_provider_healthy` is unaffected either way — it - never reads from the store. + unreachable, an affected scope's request/token/cost/provider counters + are **skipped entirely** for that scrape rather than reported as `0` — + a temporary gap in the series is the honest signal; a fabricated `0` + would read to Prometheus as a counter reset followed by a phantom spike + the instant the store recovers. This holds regardless of + `redis.failOpen`: with `failOpen: false` (or unset) the request itself + is refused during the outage, and with `failOpen: true` (the default) + requests keep flowing against the in-process fallback, but `/metrics` + still treats the affected scopes as unreadable rather than presenting + the fallback's own, non-authoritative zero as real gateway-wide usage. + `llmgateway_provider_healthy` is unaffected either way — it never reads + from the store. ## MCP and A2A @@ -1586,6 +1620,11 @@ plugin's config. envelope. A successful attempt logs the resolved user's name and the route. Neither line ever includes the presented API key — only the identity it resolved to, never the key material itself. +- **`GET /metrics` (see [Metrics](#metrics)) exposes per-user and + per-group cost and usage data**, the identical sensitivity as the admin + dashboard's JSON API, so it is gated the same way: an admin bearer + token by default, with an optional, narrowly-scoped `allowedCIDRs` + bypass for a co-located scraper — never leave it reachable with neither. ## Anonymous usage reporting @@ -1741,8 +1780,11 @@ struct — every field in that example round-trips through `New()` cleanly), a dummy backing `Deployment`/`Service` (Traefik requires a real router backend even though this middleware never actually reaches it — see [Quickstart](#quickstart)), an `IngressRoute` wiring it all to a -host, and a Traefik Helm values snippet for loading the plugin and -wiring provider keys in from Secrets via environment variables. +host, a second, Host-free `IngressRoute` plus a `VMServiceScrape` for +scraping [`/metrics`](#metrics) (a `ServiceMonitor` works the same way on +a Prometheus-operator cluster — only the CRD kind differs), and a Traefik +Helm values snippet for loading the plugin and wiring provider keys in +from Secrets via environment variables. The short version: @@ -1758,6 +1800,11 @@ The short version: - Point `redis.address` at your cluster's existing Redis/Valkey/Dragonfly rather than standing up a dedicated instance for this plugin — the distributed limiter is a client, not a datastore. +- [`/metrics`](#metrics) is served by Traefik itself, not by any backend + Service, so scraping it needs its own Host-free route (see the example + manifest's own comment on this — a Host-restricted router alone 404s an + in-cluster scrape) and its own CIDR/admin-key auth — never point a + scraper at it without one or the other configured. ### Config hot-reload diff --git a/limits.go b/limits.go index 25750db..9f05ced 100644 --- a/limits.go +++ b/limits.go @@ -871,6 +871,29 @@ func (l *limiter) storeLatched() bool { return !l.lastStoreFailure.IsZero() && l.now().Sub(l.lastStoreFailure) < storeDownLatchFor } +// configuredStoreDown reports whether l has a configured store that is +// currently within its failure latch (storeLatched) — meaning any read +// made right now is served from the in-process fallback, never the +// real, shared store, REGARDLESS of what storeGetMulti's own ok return +// says. l.store == nil (no store configured at all — a fallback-only +// deployment) always reports false: there, the fallback IS the source +// of truth by design, nothing has degraded. +// +// This exists because storeGetMulti's ok=true does not distinguish "the +// store answered for real" from "the store is down, failOpen fell back +// to the empty in-process fallback, which answered with zeros" — and +// failOpen defaults to true (newConfiguredLimiter, llmgateway.go). A +// caller that only checks ok, as currentUsage/providerUsage's callers +// used to, reports a fail-OPEN store outage as confirmed real usage: +// exactly the fabricated-counter-reset bug metrics.go's storeDown skip +// was built to prevent, except on the DEFAULT config, where it was +// never actually reachable (review fix, adversarial verification +// 2026-08-23) — see currentUsage/providerUsage's own doc comments for +// where this is now checked. +func (l *limiter) configuredStoreDown() bool { + return l.store != nil && l.storeLatched() +} + // recordStoreFailure logs err (rate-limited, see logStoreError) and opens // the store-down latch: every storeIncrBy/storeGet call for the next // storeDownLatchFor skips the network call and applies the fail-open/ @@ -1819,7 +1842,10 @@ func (l *limiter) providerUsage(scopes []limitScope) []providerCounters { } vals, ok := l.storeGetMulti(allKeys) - if !ok || len(vals) != len(allKeys) { + // configuredStoreDown catches the fail-open case ok alone misses + // (review fix, adversarial verification 2026-08-23) — see + // currentUsage's identical check and its own doc comment for why. + if !ok || len(vals) != len(allKeys) || l.configuredStoreDown() { for i := range out { out[i] = providerCounters{storeDown: true} } @@ -1906,6 +1932,14 @@ func usageWindowKeys(sc limitScope, now time.Time) []string { // every enforcement read already does, and it is read-only: unlike // checkAndCount, it never increments anything. // +// storeDown is also set when configuredStoreDown reports the store +// currently latched (review fix, adversarial verification 2026-08-23): +// with failOpen true (the default), storeGetMulti's own ok stays true +// on a store outage — it silently reads the in-process fallback instead +// — so ok alone is not enough to tell a real reading apart from a +// fail-open one served from an empty fallback. Checked in addition to, +// never instead of, storeGetMulti's own ok/length checks below. +// // ONE storeGetMulti call for the whole scopes slice (v0.2 final review // wave, 2026-08-20; supersedes the "one call per scope" amendment this // comment previously described, 2026-08-20 review): every scope's keys @@ -1947,7 +1981,7 @@ func (l *limiter) currentUsage(scopes []limitScope) []scopeUsage { // guarded defensively so a future or test-only store's short slice // reports every scope storeDown instead of panicking on an // out-of-range index below (review sweep, 2026-08-20). - if !ok || len(vals) != len(allKeys) { + if !ok || len(vals) != len(allKeys) || l.configuredStoreDown() { for i, sc := range scopes { out[i] = scopeUsage{kind: sc.kind, id: sc.id, storeDown: true} } diff --git a/metrics.go b/metrics.go index 723fe46..2f40a21 100644 --- a/metrics.go +++ b/metrics.go @@ -7,6 +7,8 @@ import ( "net/http" "sort" "strconv" + "strings" + "unicode/utf8" ) // MetricsConfig configures the Prometheus text-exposition endpoint @@ -178,41 +180,94 @@ func (g *Gateway) serveMetrics(w http.ResponseWriter) { // double quote becomes \", and newline becomes \n — the exact rule the // format requires. An unescaped quote or embedded newline in a label // value (a user or group name, a model id, an operator picks these) would -// otherwise emit an unparseable exposition document — and Prometheus -// reports an unparseable scrape as the WHOLE target being DOWN, not as -// one bad line, so this is a correctness requirement, not a formatting -// nicety. +// otherwise emit an unparseable exposition document, which is fatal to a +// real scrape — see the CORRECTNESS section below for exactly how, and +// for a claim this comment previously made that turned out to be wrong. // -// This walks s BYTE by byte and classifies each one independently, never -// a sequential find-and-replace pass: replacing quotes first and -// backslashes second (or vice versa) would double-escape a backslash -// this function itself just inserted — a real, classic bug class for -// this exact kind of escaping. A single forward pass has no such -// ordering hazard. +// Two independent correctness properties, both required, are in tension +// here and this function's whole shape exists to satisfy both at once: // -// Byte-oriented, deliberately NOT `for _, r := range s` (review fix, -// adversarial verification 2026-08-23): ranging over a string decodes -// UTF-8 and substitutes U+FFFD for every invalid byte, which makes the -// function NOT injective — two distinct configured names that both -// contain invalid UTF-8 (a byte sequence auth.go's buildEntry never -// rejects; it validates non-empty only, see its own doc comment) can -// decode to the identical U+FFFD run and render as the SAME escaped -// label value. Two scope_id values colliding onto one series is a -// duplicate-series scrape failure — confirmed against a real Prometheus -// scrape parser: the whole target reports DOWN, and promtool alone will -// not catch it, since expfmt silently dedupes. Escaping byte-for-byte -// passes every raw byte through unchanged except the three ASCII -// characters requiring escaping ('\\', '"', '\n') — none of which can -// ever appear as a UTF-8 continuation byte (continuation bytes are -// always >= 0x80), so this never misinterprets a multi-byte sequence's -// internal bytes as one of these three, and every distinct input byte -// string still maps to a distinct output. It also avoids WriteRune's -// per-rune decode cost on a large value. +// 1. INJECTIVE: two distinct inputs must never produce the same escaped +// output. A collision means two distinct scope_id/model/provider +// values render as the identical label value — a duplicate series. +// 2. VALID UTF-8 OUT: the output must always be valid UTF-8, regardless +// of whether s is. +// +// CORRECTNESS (review fixes, adversarial verification 2026-08-23, two +// rounds): round 1 walked s byte-by-byte and copied every non-special +// byte through verbatim, including an invalid one — injective (property +// 1 held), but an invalid byte copied through means the OUTPUT can itself +// be invalid UTF-8, which fails property 2. Measured this round against +// prometheus/prometheus's real scrape path (promparse.go): an invalid- +// UTF-8 label value is FATAL — the parser errors, the scrape loop breaks, +// and Prometheus marks the target up=0 (DOWN). That is a strictly worse +// failure than the one round 1 was fixing: the doc comment at the time +// claimed a duplicate series is ALSO fatal ("the whole target reports +// DOWN"); measured properly this round against scrape.go, that claim was +// wrong — checkAddError treats a duplicate sample as non-fatal (returns +// false, nil), bumps a counter, and the scrape still succeeds with +// up=1. So the ORIGINAL rune-based version (pre-round-1) was lossy but +// safe: colliding invalid-UTF-8 inputs onto one U+FFFD-bearing series +// dropped data but never took the target down. Round 1's byte-oriented +// version was lossless but unsafe: correct in the one dimension it +// measured, worse in the one it did not. +// +// This version keeps both: valid runes (including a genuine, already- +// valid 3-byte-encoded U+FFFD that was actually present in s) pass +// through unchanged via utf8.DecodeRuneInString, which reports both the +// rune and how many bytes it consumed; only a TRULY undecodable single +// byte (RuneError with a reported width of 1 — DecodeRuneInString's own +// documented signal that byte could not start any valid encoding, as +// opposed to width 3 for a real U+FFFD) is replaced with \xHH, HH being +// that byte's own hex value — recoverable, so distinct invalid bytes can +// never collide, and it can never appear from the normal escape path +// either: every literal backslash in s is ASCII and therefore always +// decodes as its own valid 1-byte rune, which the switch below always +// doubles to \\ — a lone, undoubled backslash in the output can +// therefore only ever be this marker, never user-typed text. Reachability +// caveat: no live config path is known to reach the invalid-UTF-8 branch +// today (encoding/json, which every user/group name currently decodes +// through, coerces invalid UTF-8 to U+FFFD before this function ever +// sees it) — this is closing a latent gap, not a currently-exploitable +// one, but the SAME property (injective, always-valid-UTF-8 output) is +// what a future caller — an upstream-reported model id read some other +// way, say — would need without re-deriving this reasoning. +// +// Fast path (item 5, adversarial verification round 2, benchmarked): +// nearly every real user/group/model name needs none of the above — no +// backslash/quote/newline, already valid UTF-8 (true of every Go string +// literal and every encoding/json-decoded value by construction) — so +// skip the walk and its allocation entirely when s is already its own +// answer. Benchmarked over 7 representative values: the byte-oriented +// slow path alone cost 318.8 ns/op, 560 B/op, 14 allocs/op (worse than +// the original rune-based version's 344.7 ns, 120 B, 7 allocs on +// allocations specifically, from bytes.Buffer.String() copying where +// strings.Builder.String() does not — fixed below by switching to +// strings.Builder too); the fast path measures 84.4 ns/op, 0 B, 0 +// allocs/op. At 1,000 configured users that is the difference between +// roughly 20,000 allocations and 800 KB of garbage per scrape, inside +// the shared Traefik process, and zero. func escapeLabelValue(s string) string { - var b bytes.Buffer + if !strings.ContainsAny(s, "\\\"\n") && utf8.ValidString(s) { + return s + } + + var b strings.Builder b.Grow(len(s)) - for i := 0; i < len(s); i++ { - switch c := s[i]; c { + for i := 0; i < len(s); { + r, size := utf8.DecodeRuneInString(s[i:]) + if r == utf8.RuneError && size == 1 { + // A genuinely undecodable byte — see this function's own + // CORRECTNESS section above for why \xHH, not � and not + // a raw copy, is the only choice that keeps both required + // properties. + b.WriteString(`\x`) + b.WriteByte(hexDigit(s[i] >> 4)) + b.WriteByte(hexDigit(s[i] & 0x0f)) + i++ + continue + } + switch r { case '\\': b.WriteString(`\\`) case '"': @@ -220,12 +275,27 @@ func escapeLabelValue(s string) string { case '\n': b.WriteString(`\n`) default: - b.WriteByte(c) + // A valid rune, 1-4 bytes: none of the three special ASCII + // characters above can appear as any byte of a valid + // multi-byte encoding (every byte of one is >= 0x80), so a + // verbatim copy of s[i:i+size] can never hide an unescaped + // backslash/quote/newline inside it. + b.WriteString(s[i : i+size]) } + i += size } return b.String() } +// hexDigit returns the lowercase hex digit for the low nibble of n +// (0x0-0xf) — escapeLabelValue's own \xHH marker for an undecodable +// byte, written without fmt.Sprintf to keep that (already-rare) branch +// allocation-free too. +func hexDigit(n byte) byte { + const digits = "0123456789abcdef" + return digits[n&0x0f] +} + // metricLabel is one label name/value pair a metricWriter sample carries. type metricLabel struct { name string @@ -323,6 +393,14 @@ const aggregationNotePerProcess = " AGGREGATION ACROSS REPLICAS: this counter li // instant (e.g. one mid-backoff, one already recovered). const aggregationNoteProviderHealthy = " AGGREGATION ACROSS REPLICAS: each replica runs its own discovery circuit breaker in-process, never shared via Redis, so this can legitimately differ per replica. Read it per-instance where possible; if you must aggregate, use min() to surface \"at least one replica sees this provider as unhealthy\" — sum() is meaningless for a 0/1 gauge." +// aggregationNoteStoreHealth is appended to llmgateway_limit_store_up's +// HELP text: whether THIS replica's own connection to the configured +// limit store is currently healthy is, by definition, discovered +// locally (limiter.storeLatched, limits.go) — it cannot itself be read +// from the store being described, so — like provider health above — +// it is inherently per-replica, never shared via Redis. +const aggregationNoteStoreHealth = " AGGREGATION ACROSS REPLICAS: each replica discovers its own connection health to the configured store independently — this cannot itself be read from the store being described — so it can legitimately differ per replica. Read it per-instance where possible; if you must aggregate, use min() to surface \"at least one replica currently sees the store as down\" — sum() is meaningless for a 0/1 gauge." + // renderMetrics builds the full Prometheus text-exposition document for // GET . Every read below goes through the SAME batched, // chunked accessors GET /admin/api/overview and GET /admin/api/usage @@ -338,6 +416,7 @@ func (g *Gateway) renderMetrics() []byte { g.writeUsageMetrics(&m) g.writeProviderMetrics(&m) g.writeRejectionMetrics(&m) + g.writeStoreHealthMetrics(&m) return m.buf.Bytes() } @@ -556,13 +635,49 @@ func (g *Gateway) writeRejectionMetrics(m *metricWriter) { }) m.family("llmgateway_rate_limit_rejections_total", "counter", - "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed — it resets only on restart."+aggregationNotePerProcess) + fmt.Sprintf( + "Total requests refused by a rate or budget limit since process start, by scope. Unlike the request/token/cost counters above, this is a process-lifetime counter, not UTC-day-windowed, but it does NOT reset only on restart: a scope's own count resets to 0 when that user is removed via a hot-reloaded users file, and every tracked scope resets together, in one bulk cliff (not a graceful per-scope decay), the instant more than %d distinct scopes have ever been seen — a value that drops is one of these events, not a scrape anomaly.", + rejectionCounterMapCap, + )+aggregationNotePerProcess) for _, s := range snaps { m.sampleInt("llmgateway_rate_limit_rejections_total", []metricLabel{{"scope_kind", s.kind}, {"scope_id", s.id}}, s.count) } } +// writeStoreHealthMetrics emits llmgateway_limit_store_up (item 7, +// adversarial verification 2026-08-23): every store-backed family above +// simply goes silent for the scopes it cannot read during an outage +// (writeUsageMetrics/writeProviderMetrics's own storeDown skip) — the +// honest choice for THOSE families (see their own doc comments), but it +// means a store outage has NO signal of its own on this endpoint: with +// failOpen false and a store flapping faster than the fail latch +// (storeDownLatchFor, limits.go), every store-backed family can simply +// be ABSENT, scrape after scrape, with nothing on /metrics itself +// saying why — an operator needs live traffic AND a fail-closed config +// to even notice. This gauge exists to be the "why are my other +// families missing" signal on its own, independent of traffic volume or +// failOpen. +// +// Absent entirely when no limit store is configured at all (a fallback- +// only deployment, limiter.redisStatus' own "configured" return) — +// there is no store health to report there; the fallback is always +// "up" by definition, a trivial, uninteresting reading not worth a +// series. +func (g *Gateway) writeStoreHealthMetrics(m *metricWriter) { + configured, _, _ := g.limiter.redisStatus() + if !configured { + return + } + up := int64(1) + if g.limiter.configuredStoreDown() { + up = 0 + } + m.family("llmgateway_limit_store_up", "gauge", + "1 when this replica's own connection to the configured limit store (Redis) is currently healthy, 0 when it is latched down after a recent failure (limiter.storeLatched, limits.go). A store-backed family (llmgateway_requests_total and the rest) going silent for one or more scrapes in a row, with this gauge at 0, is the store outage those families' own storeDown skip is deliberately quiet about."+aggregationNoteStoreHealth) + m.sampleInt("llmgateway_limit_store_up", nil, up) +} + // pruneRejectionsAfterReload evicts every "user"-kind rejection scope // (limiter.pruneRejections, limits.go) whose name is no longer among // g.auth's currently active users — ServeHTTP's own call site diff --git a/metrics_test.go b/metrics_test.go index 08214a5..efd9055 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" "time" + "unicode/utf8" ) // newMetricsTestConfig builds a Config with Metrics enabled at the default @@ -386,7 +387,12 @@ func TestMetrics_OutputParsesAsValidExposition(t *testing.T) { cfg.Metrics.ModelLabel = true cfg.Providers["openai"].BaseURL = srv.URL cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerMinute: 1} - h, _ := newMetricsGatewayHandle(t, cfg) + h, gw := newMetricsGatewayHandle(t, cfg) + // A configured (if in-process) store, so llmgateway_limit_store_up + // (item 7) also has a sample to check structure against below — + // newMemoryStore behaves identically to the fallback it stands in + // for, so this changes nothing else about this test's traffic. + gw.limiter.store = newMemoryStore() body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} // First request: admitted, and succeeds against the local httptest @@ -462,6 +468,7 @@ func TestMetrics_OutputParsesAsValidExposition(t *testing.T) { "llmgateway_provider_model_attempts_total", "llmgateway_provider_model_failures_total", "llmgateway_rate_limit_rejections_total", + "llmgateway_limit_store_up", } for _, name := range wantFamilies { if _, ok := types[name]; !ok { @@ -487,12 +494,20 @@ func TestEscapeLabelValue(t *testing.T) { {"backslash_then_quote", `\"`, `\\\"`}, {"all_three", "a\"b\\c\nd", `a\"b\\c\nd`}, // Invalid UTF-8 bytes (0x80/0x81 are bare continuation bytes, - // never valid as the START of any UTF-8 sequence) must pass - // through UNCHANGED, byte-for-byte — see - // TestEscapeLabelValue_InvalidUTF8_Injective below for why this - // matters beyond just "does not crash". - {"invalid_utf8_lone_0x80", "u\x80", "u\x80"}, - {"invalid_utf8_lone_0x81", "u\x81", "u\x81"}, + // never valid as the START of any UTF-8 sequence) must become a + // deterministic, recoverable \xHH marker — see + // TestEscapeLabelValue_InvalidUTF8_InjectiveAndValidUTF8 below for + // why this is the only choice satisfying both required + // properties (injective, and always valid UTF-8 out). + {"invalid_utf8_lone_0x80", "u\x80", `u\x80`}, + {"invalid_utf8_lone_0x81", "u\x81", `u\x81`}, + // A literal, user-typed "\x80" (five ordinary ASCII characters, + // not a raw invalid byte) must render distinguishably from the + // marker above: its own backslash gets doubled like any other, + // producing \\x80 (six characters) rather than colliding with + // the four-character \x80 marker an actual invalid byte 0x80 + // produces. + {"literal_backslash_x80_text", `\x80`, `\\x80`}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -504,27 +519,71 @@ func TestEscapeLabelValue(t *testing.T) { } } -// TestEscapeLabelValue_InvalidUTF8_Injective is the mutation-proofing -// test for the byte-oriented rewrite (review fix, adversarial -// verification 2026-08-23): a rune-oriented `for _, r := range s` -// implementation decodes invalid UTF-8 to U+FFFD, which makes escaping -// NOT injective — "u\x80" and "u\x81" are two distinct, real inputs -// (auth.go's buildEntry validates a user name as non-empty only, never -// restricted to valid UTF-8) that would both render as "u�" under -// that approach, colliding two distinct scope_id label values onto one -// series — confirmed against a real Prometheus scrape parser: a -// duplicate series, and the whole scrape target reported DOWN. The -// byte-oriented implementation must keep every input byte-for-byte -// distinguishable. -func TestEscapeLabelValue_InvalidUTF8_Injective(t *testing.T) { +// TestEscapeLabelValue_InvalidUTF8_InjectiveAndValidUTF8 is the +// mutation-proofing test for the two-round rewrite (review fixes, +// adversarial verification 2026-08-23, round 2 correcting round 1): +// +// - Round 1 fixed a rune-oriented `for _, r := range s` implementation, +// which decodes invalid UTF-8 to U+FFFD and is therefore NOT +// injective: "u\x80" and "u\x81" both render as "u�", colliding two +// distinct scope_id values onto one series (a duplicate series). +// - Round 1's own fix (copying an invalid byte through raw) achieved +// injectivity but broke the OTHER required property: the OUTPUT is +// then itself invalid UTF-8. Measured against prometheus/prometheus's +// real scrape parser (promparse.go) this round: that is FATAL — the +// scrape errors out entirely (target reported DOWN), which is worse +// than a duplicate series (measured non-fatal against scrape.go: +// dropped, target stays up). Round 1's own doc comment asserted the +// opposite — that a duplicate series is what takes a target down — +// which was the wrong premise, corrected here. +// +// This test pins both properties at once, since a fix for either one +// alone (as both prior rounds separately proved) is not sufficient. +func TestEscapeLabelValue_InvalidUTF8_InjectiveAndValidUTF8(t *testing.T) { t.Parallel() a := escapeLabelValue("u\x80") b := escapeLabelValue("u\x81") if a == b { - t.Fatalf("escapeLabelValue(%q) == escapeLabelValue(%q) == %q — two distinct inputs collided onto one label value", "u\x80", "u\x81", a) + t.Fatalf("escapeLabelValue(%q) == escapeLabelValue(%q) == %q — two distinct inputs collided onto one label value (not injective)", "u\x80", "u\x81", a) } - if strings.Contains(a, "�") || strings.Contains(b, "�") { - t.Errorf("escaped output contains U+FFFD (replacement character) — the rune-decoding bug this test guards against: a=%q b=%q", a, b) + if !utf8.ValidString(a) { + t.Errorf("escapeLabelValue(%q) = %q is not valid UTF-8 — fatal to a real Prometheus scrape (promparse.go), not merely a duplicate series", "u\x80", a) + } + if !utf8.ValidString(b) { + t.Errorf("escapeLabelValue(%q) = %q is not valid UTF-8 — fatal to a real Prometheus scrape (promparse.go), not merely a duplicate series", "u\x81", b) + } +} + +// TestEscapeLabelValue_Exhaustive1And2Byte_InjectiveAndValidUTF8 proves +// both required properties by construction rather than by spot-check, +// over every possible 1-byte string (256) and every possible 2-byte +// string (65,536) — valid UTF-8, invalid UTF-8, and every ASCII special +// character alike — all checked against ONE shared map, so a collision +// BETWEEN a 1-byte and a 2-byte input (the escaped output is not +// length-preserving, so this is not automatically ruled out just +// because the inputs differ in length) would be caught too, not only a +// same-length collision. Fast enough (well under a second) to run on +// every test invocation, not just as a one-off manual verification. +func TestEscapeLabelValue_Exhaustive1And2Byte_InjectiveAndValidUTF8(t *testing.T) { + t.Parallel() + seen := make(map[string]string, 256+65536) + check := func(in string) { + out := escapeLabelValue(in) + if !utf8.ValidString(out) { + t.Fatalf("escapeLabelValue(%q) = %q is not valid UTF-8", in, out) + } + if prior, ok := seen[out]; ok && prior != in { + t.Fatalf("collision: escapeLabelValue(%q) and escapeLabelValue(%q) both produced %q", prior, in, out) + } + seen[out] = in + } + for b := 0; b < 256; b++ { + check(string([]byte{byte(b)})) + } + for a := 0; a < 256; a++ { + for b := 0; b < 256; b++ { + check(string([]byte{byte(a), byte(b)})) + } } } @@ -759,13 +818,26 @@ func TestMetrics_ProviderHealthy_ReflectsOpenBreaker(t *testing.T) { // proofing test for every `if su.storeDown { continue }` / // `if mc.storeDown { continue }` guard in metrics.go: deleting any of // them makes this test fail. Forces the SAME limiter every family in -// this file reads from into a fail-closed store outage (alwaysErrStore, -// failOpen=false — routes_unified_test.go's own stub), then asserts -// every store-backed family emits NO samples at all rather than a -// fabricated 0 (review fix, adversarial verification 2026-08-23: -// without the guards, a Redis blip reads as a hard 0 mid-outage, then -// jumps back to the real total on recovery — Prometheus treats that as -// a counter reset followed by a spurious full-total "increase"). +// this file reads from into a store outage — a real, configured store +// (alwaysErrStore, routes_unified_test.go's own stub) that errors on +// every call — then asserts every store-backed family emits NO samples +// at all rather than a fabricated 0 (review fix, adversarial +// verification 2026-08-23: without the guards, a Redis blip reads as a +// hard 0 mid-outage, then jumps back to the real total on recovery — +// Prometheus treats that as a counter reset followed by a spurious +// full-total "increase"). +// +// Deliberately does NOT touch gw.limiter.failOpen (round 2, adversarial +// verification 2026-08-23): newConfiguredLimiter (llmgateway.go) +// defaults it to true, and a round-1 version of this test hand-set it +// to false, which meant the suite only ever exercised the fail-CLOSED +// path — the one case where storeGetMulti's own ok=false already made +// the bug impossible to miss. The actual production default (failOpen +// true) silently read zeros from the empty in-process fallback and +// reported ok=true, bypassing every storeDown guard in this file +// entirely; that is the path this test now pins, matching +// limiter.configuredStoreDown's own doc comment (limits.go). +// // llmgateway_provider_healthy is deliberately excluded from the "must be // absent" list: it comes from the in-process discovery breaker, never // the counterStore, so a store outage must NOT affect it — asserted @@ -777,8 +849,13 @@ func TestMetrics_StoreDown_SkipsRatherThanFabricatesZero(t *testing.T) { cfg.Users.Inline[0].Limits = &LimitsConfig{RequestsPerDay: 100} h, gw := newMetricsGatewayHandle(t, cfg) + // Simulates "Redis was configured and reachable, then broke" without + // standing up a real broken Redis connection: gw.limiter.store starts + // nil (no cfg.Redis in newMetricsTestConfig) and is forced non-nil + // here, exactly mirroring TestLimiterCurrentUsage_StoreDown's own + // direct-field-injection style (admin_test.go). failOpen is left + // untouched at its true default. gw.limiter.store = alwaysErrStore{} - gw.limiter.failOpen = false rec := httptest.NewRecorder() h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) @@ -814,6 +891,50 @@ func TestMetrics_StoreDown_SkipsRatherThanFabricatesZero(t *testing.T) { if !providerHealthyPresent { t.Error("llmgateway_provider_healthy missing — this family reads in-process breaker state, not the store, and must still render during a store outage") } + + // llmgateway_limit_store_up (item 7, adversarial verification + // 2026-08-23) is the one signal that IS supposed to go to 0 here — + // it exists specifically to explain why every family above just + // went silent. + assertSample(t, samples, "llmgateway_limit_store_up", nil, "0") +} + +// --- store health gauge --- + +// TestMetrics_LimitStoreUp_AbsentWhenNoStoreConfigured proves the gauge +// is not emitted at all for a fallback-only deployment (no cfg.Redis) — +// there is no store health to report there. +func TestMetrics_LimitStoreUp_AbsentWhenNoStoreConfigured(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() // no cfg.Redis + h, _ := newMetricsGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if strings.Contains(rec.Body.String(), "llmgateway_limit_store_up") { + t.Error("llmgateway_limit_store_up present with no store configured, want absent entirely") + } +} + +// TestMetrics_LimitStoreUp_ReflectsHealthyStore proves the gauge reads 1 +// for a configured, currently-healthy store — newMemoryStore, not +// alwaysErrStore, standing in for "a real store that is up": it +// implements counterStore and never errors, so l.store is non-nil +// (configured) while l.storeLatched() stays false (never failed). +func TestMetrics_LimitStoreUp_ReflectsHealthyStore(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, gw := newMetricsGatewayHandle(t, cfg) + + gw.limiter.store = newMemoryStore() + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + _, samples := parsePrometheusText(t, rec.Body.Bytes()) + assertSample(t, samples, "llmgateway_limit_store_up", nil, "1") } // --- rate-limit rejections --- @@ -931,6 +1052,48 @@ func TestMetrics_PruneRejectionsAfterReload_EndToEnd(t *testing.T) { } } +// TestServeHTTP_PruneRejectionsOnlyAfterRealReload is the mutation- +// proofing counterpart to TestMetrics_PruneRejectionsAfterReload_ +// EndToEnd above: that test proves pruning DOES happen on a real +// reload; this one proves it does NOT happen otherwise (review fix, +// adversarial verification 2026-08-23 — replacing `if g.auth. +// maybeReload() { g.pruneRejectionsAfterReload() }` in ServeHTTP +// (llmgateway.go) with two unconditional statements survived the whole +// suite until this test existed). +// +// newMetricsTestConfig configures no Users.File at all, so authStore. +// maybeReload's very first check (usersFile == nil) makes it return +// false on every one of the 200 requests below — a real reload never +// happens. Seeds a rejection entry for "ghost", a name that is not any +// currently active user: pruneRejectionsAfterReload would delete it +// (it is not in authStore's own snapshot) the instant it ever ran, so +// its survival after 200 ordinary requests is the pinned guarantee. +func TestServeHTTP_PruneRejectionsOnlyAfterRealReload(t *testing.T) { + t.Parallel() + cfg := newMetricsTestConfig() + h, gw := newMetricsGatewayHandle(t, cfg) + + gw.limiter.rejections.increment("user", "ghost") + + for i := 0; i < 200; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, metricsRequest(http.MethodGet, metricsPathDefault, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("request %d: status = %d, want 200", i, rec.Code) + } + } + + var found bool + for _, s := range gw.limiter.rejectionSnapshot() { + if s.kind == "user" && s.id == "ghost" { + found = true + } + } + if !found { + t.Error(`the "ghost" rejection entry was pruned despite no real reload ever happening — pruneRejectionsAfterReload must run only when authStore.maybeReload reports a real reload, never unconditionally on every request`) + } +} + // --- budget-consumed ratio --- func TestMetrics_BudgetConsumedRatio_ReflectsUsageVsLimit(t *testing.T) {