fix(limits): never fabricate zero counters, bound the rejection counter

Two correctness fixes from adversarial verification.

providerUsage returned every counter at its zero value on a
failed-closed store read, with no signal distinguishing that from a
real zero. metrics.go rendered the zero as a hard value. A single
Redis blip therefore read as a counter reset, and the recovery scrape
counted the whole prior total as a fresh increase, painting a phantom
multi-thousand-request spike from one transient error. Add storeDown
to providerCounters and set it on the failed-closed path, mirroring
scopeUsage's existing contract.

limiter.rejections grew without bound: nothing ever removed a scope
whose user was renamed or deleted from a hot-reloaded users file, so a
long-running process kept exporting a series for every name ever seen.
Bound the map with the same current/previous generation-rotation
authFailureTracker already uses, and evict a user's entry when
authStore.maybeReload performs a real reload. maybeReload now returns
whether it actually reloaded, so ServeHTTP only runs the eviction walk
on that rare event, never on every request.

Also corrects a stale comment: the rejection counter's store-down
branch fires on every request during a full outage, not rarely, as an
earlier comment claimed.

limits.go

@@ -593,10 +593,20 @@ type limiter struct {
 	// requests were rejected" has no existing counter to read at all
 	// (req:min/req:day count every admission attempt, accepted or not,
 	// by design — see checkAndCount's own doc comment). Adding it here
-	// as a bare map increment costs one uncontended lock per rejection
-	// (already the rare, not-hot-path outcome of checkAndCount), never a
-	// second network round trip, and never re-derives a number the
-	// store already tracks.
+	// as a bare map increment costs one uncontended lock per rejection,
+	// never a second network round trip, and never re-derives a number
+	// the store already tracks.
+	//
+	// CORRECTION (adversarial verification, 2026-08-23): an earlier
+	// version of this comment called the increment "the rare, not-hot-
+	// path outcome of checkAndCount" — true for an ordinary single-scope
+	// limit breach, but false for the storeDownViolation branch
+	// (checkAndCount's own body, below): with a configured store down
+	// and failOpen false, EVERY request takes that branch, so the
+	// increment runs on every request for as long as the outage lasts,
+	// not rarely. The honest claim is narrower: it is still cheaper than
+	// the network round trip that already failed to produce this
+	// violation in the first place, never an ADDITIONAL one.
 	rejections rejectionCounter
 	logMu      sync.Mutex
 	failOpen   bool // store-error policy: true falls back to fallback, false refuses the request
@@ -613,31 +623,67 @@ type rejectionScope struct {
 	id   string
 }
 
+// rejectionCounterMapCap bounds how many distinct (kind, id) scopes
+// rejectionCounter.current tracks before rotating (review fix,
+// adversarial verification 2026-08-23): replaceFileUsers (auth.go) can
+// hot-reload the configured user/group set at any time, and nothing
+// here ever removed a scope's entry when the user or group behind it
+// was renamed or deleted — over a long-running process's lifetime, a
+// deployment that renames or churns users keeps exporting a series FOR
+// EVERY NAME EVER SEEN, not just currently configured ones, growing
+// this map (and the resulting Prometheus series count) without bound.
+// 2000 comfortably exceeds the 1,000-user scale this codebase already
+// treats as a real deployment size elsewhere (adminUsageChunkScopes'
+// own doc comment, admin.go) with headroom left for renamed-user churn
+// and the synthetic store_down entry, while still bounding the worst
+// case.
+const rejectionCounterMapCap = 2000
+
 // rejectionCounter accumulates rate/budget-limit-violation counts per
 // scope for the lifetime of the process — limiter.rejections' own
 // backing store; see its doc comment for why this exists as a bare
-// in-process map rather than a counterStore key. Unlike authFailureTracker
-// (auth.go), there is no TTL or generation rotation here: a Prometheus
-// counter must only ever grow for as long as the process runs, so there
-// is nothing to roll over, and cardinality is bounded by the configured
-// user/group count (checkAndCount only ever sees scopes for an already-
-// authenticated caller's own user/group, never attacker-controlled
-// strings), the same bound scopeUsage's own per-scope metrics already
-// carry.
+// in-process map rather than a counterStore key.
+//
+// current/previous generation-rotation (review fix, adversarial
+// verification 2026-08-23) mirrors authFailureTracker's own shape
+// (auth.go) for the identical reason: bound total memory without ever
+// scanning. Rotation is a two-pointer swap (O(1)), triggered on
+// inserting a brand-new scope once current is already at
+// rejectionCounterMapCap; total memory is bounded to at most
+// 2×rejectionCounterMapCap entries. Unlike authFailureTracker, there is
+// no TTL — a Prometheus counter must only ever grow for as long as its
+// series is being reported, so a scope's count is never read back out
+// of previous once superseded (see snapshot, below): a scope displaced
+// by rotation, if it is ever rejected again, starts a fresh count at 1
+// rather than resuming its old total. That is a real, visible counter
+// reset for that one series — accepted, the same trade-off
+// authFailureTracker's own doc comment already accepts for a different
+// bounded resource, and one Prometheus's own counter-reset handling in
+// rate()/increase() already tolerates (the identical mechanism that
+// already tolerates a process restart).
 type rejectionCounter struct {
-	counts map[rejectionScope]int64
-	mu     sync.Mutex
+	current  map[rejectionScope]int64
+	previous map[rejectionScope]int64
+	mu       sync.Mutex
 }
 
-// increment records one rejection for (kind, id), initializing the
-// backing map on first use — rejectionCounter's zero value (as embedded,
-// unexported, in limiter) is ready to use without a constructor.
+// increment records one rejection for (kind, id), initializing current
+// on first use — rejectionCounter's zero value (as embedded, unexported,
+// in limiter) is ready to use without a constructor. Rotates current ->
+// previous before inserting a brand-new (kind, id) once current is
+// already at rejectionCounterMapCap, mirroring authFailureTracker.
+// increment's identical shape (auth.go).
 func (c *rejectionCounter) increment(kind, id string) {
+	key := rejectionScope{kind: kind, id: id}
 	c.mu.Lock()
-	if c.counts == nil {
-		c.counts = make(map[rejectionScope]int64)
+	if c.current == nil {
+		c.current = make(map[rejectionScope]int64)
 	}
-	c.counts[rejectionScope{kind: kind, id: id}]++
+	if _, ok := c.current[key]; !ok && len(c.current) >= rejectionCounterMapCap {
+		c.previous = c.current
+		c.current = make(map[rejectionScope]int64, rejectionCounterMapCap)
+	}
+	c.current[key]++
 	c.mu.Unlock()
 }
 
@@ -649,18 +695,43 @@ type rejectionSnapshot struct {
 	count int64
 }
 
-// snapshot returns every scope with at least one recorded rejection, in
-// no particular order (metrics.go sorts its own copy before rendering).
+// snapshot returns every scope in current with at least one recorded
+// rejection, in no particular order (metrics.go sorts its own copy
+// before rendering). previous is deliberately NOT merged in: a scope
+// there was displaced by rotation, and re-reporting its stale, no-
+// longer-growing count would misrepresent it as still current.
 func (c *rejectionCounter) snapshot() []rejectionSnapshot {
 	c.mu.Lock()
 	defer c.mu.Unlock()
-	out := make([]rejectionSnapshot, 0, len(c.counts))
-	for scope, n := range c.counts {
+	out := make([]rejectionSnapshot, 0, len(c.current))
+	for scope, n := range c.current {
 		out = append(out, rejectionSnapshot{kind: scope.kind, id: scope.id, count: n})
 	}
 	return out
 }
 
+// pruneUserScopes deletes every "user"-kind entry from both current and
+// previous whose id is not in keep — limiter.pruneRejections' own
+// backing implementation; see that method's doc comment for when and
+// why this runs. Walking and deleting from a live map mid-range is safe
+// in Go (deleting the current key during a range is explicitly
+// permitted); previous is included so a recently-rotated, still-stale
+// entry for a deleted user does not linger there either.
+func (c *rejectionCounter) pruneUserScopes(keep map[string]bool) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	for scope := range c.current {
+		if scope.kind == "user" && !keep[scope.id] {
+			delete(c.current, scope)
+		}
+	}
+	for scope := range c.previous {
+		if scope.kind == "user" && !keep[scope.id] {
+			delete(c.previous, scope)
+		}
+	}
+}
+
 // rejectionScopeStoreDown is the synthetic scope id checkAndCount
 // records a rejection under when it refuses a request because the
 // configured store is unreachable and failOpen is false
@@ -678,6 +749,21 @@ func (l *limiter) rejectionSnapshot() []rejectionSnapshot {
 	return l.rejections.snapshot()
 }
 
+// pruneRejections removes every "user"-kind rejection scope whose id is
+// not in keep (review fix, adversarial verification 2026-08-23): a user
+// removed from a hot-reloaded users file (auth.go's replaceFileUsers)
+// must not keep exporting llmgateway_rate_limit_rejections_total
+// forever. Called only from ServeHTTP, and only when authStore.
+// maybeReload just reported a REAL reload — see that method's own doc
+// comment (users_file.go) for why this must never run on every request.
+// Scope kinds other than "user" (group, the synthetic total and
+// store_down entries) are left untouched: groups and the total scope
+// are never hot-reloaded (Config.Groups is fixed at construction), and
+// store_down names no real scope at all.
+func (l *limiter) pruneRejections(keep map[string]bool) {
+	l.rejections.pruneUserScopes(keep)
+}
+
 // providerAttemptSpawnCap bounds how many concurrent recordProviderAttempt
 // store writes one limiter's production spawn implementation will run at
 // once (FOLDED-1, v0.22 review round, round 2) — see limiter.spawnTokens'
@@ -1677,6 +1763,20 @@ type providerCounters struct {
 	failuresMinute int64
 	attemptsDay    int64
 	failuresDay    int64
+	// storeDown reports whether reading this scope's counters failed
+	// closed (a configured store errored and failOpen is false) —
+	// mirrors scopeUsage.storeDown exactly (review fix: the doc comment
+	// below used to claim "nothing downstream treats a provider's zero
+	// counters as confirmed no-traffic", which metrics.go's renderer
+	// made false the moment it existed — a Redis blip made every
+	// provider counter read as 0, then jump back to its real value on
+	// the next successful scrape, which Prometheus reads as a counter
+	// RESET followed by a fresh increase equal to the whole prior total:
+	// one transient store error paints a phantom multi-thousand-request
+	// spike. Every field is 0 when this is true; a caller must not
+	// present them as "confirmed zero usage", the identical rule
+	// scopeUsage's own doc comment states).
+	storeDown bool
 }
 
 // providerUsage reads every scope's current attempt/failure counters in
@@ -1688,11 +1788,16 @@ type providerCounters struct {
 // kindProviderModel entries for the per-model breakdown, all in one
 // slice, so both share this single round trip. Order is preserved:
 // providerUsage(scopes)[i] corresponds to scopes[i]. A failed read
-// (store down + fail-closed) returns every zero-value counters, exactly
-// like targetUsage — see its own doc comment for why that is acceptable
-// here: nothing downstream treats a provider's zero counters as
-// "confirmed no traffic" the way scopeUsage.storeDown guards against for
-// a limited scope.
+// (store down + fail-closed) returns every counter at its zero value
+// WITH storeDown set (review fix: adversarial verification, 2026-08-23
+// — the previous version of this comment claimed nothing downstream
+// treats a provider's zero counters as confirmed no-traffic; metrics.go
+// is exactly such a downstream, and without this flag a transient Redis
+// blip fabricated a hard 0 that Prometheus reads as a counter reset,
+// followed by a fresh "increase" equal to the whole prior total on the
+// next successful scrape). A caller reading providerCounters must skip
+// a storeDown entry the same way currentUsage's own callers already skip
+// a storeDown scopeUsage — see that type's doc comment.
 //
 // Slicing is by running offset, not a fixed i*stride multiply (SHOULD-2,
 // providerCounterKeysPerScope's own doc comment): a kindProvider and a
@@ -1715,7 +1820,10 @@ func (l *limiter) providerUsage(scopes []limitScope) []providerCounters {
 
 	vals, ok := l.storeGetMulti(allKeys)
 	if !ok || len(vals) != len(allKeys) {
-		return out // zero-value counters; see doc comment above
+		for i := range out {
+			out[i] = providerCounters{storeDown: true}
+		}
+		return out
 	}
 
 	for i, sc := range scopes {

limits_test.go

@@ -1944,14 +1944,107 @@ func TestProviderUsage_EmptyScopes_ReturnsEmptyWithoutTouchingStore(t *testing.T
 
 // TestProviderUsage_StoreDown_ReturnsZeroCounters mirrors targetUsage's
 // own fail-closed contract: a store error with failOpen=false must report
-// every scope's counters as zero, not partial or stale values.
+// every scope's counters as zero, not partial or stale values, AND must
+// set storeDown (review fix, adversarial verification 2026-08-23): a
+// caller that cannot tell a real zero from a failed read would otherwise
+// fabricate a counter-reset spike in metrics.go's rendering.
 func TestProviderUsage_StoreDown_ReturnsZeroCounters(t *testing.T) {
 	l := newLimiter(alwaysErrStore{}, false)
 	got := l.providerUsage([]limitScope{{kind: kindProvider, id: "openai"}})
 	if len(got) != 1 {
 		t.Fatalf("len(got) = %d, want 1", len(got))
 	}
-	if got[0] != (providerCounters{}) {
-		t.Errorf("counters = %+v, want the zero value on a storeDown read", got[0])
+	if !got[0].storeDown {
+		t.Errorf("counters = %+v, want storeDown true on a failed-closed read", got[0])
+	}
+	if got[0] != (providerCounters{storeDown: true}) {
+		t.Errorf("counters = %+v, want every non-storeDown field at its zero value", got[0])
+	}
+}
+
+// --- rejectionCounter: bounding, rotation, pruning (review fixes, adversarial verification 2026-08-23) ---
+
+// TestRejectionCounter_BoundsMemoryViaRotation mirrors
+// TestAuthFailureTracker_BoundsMemoryViaRotation (auth_test.go) exactly:
+// filling current past rejectionCounterMapCap distinct scopes must
+// trigger an O(1) generation rotation, not unbounded growth — a
+// deployment that renames or churns users over a long process lifetime
+// must never grow this map (and the resulting Prometheus series count)
+// without bound.
+func TestRejectionCounter_BoundsMemoryViaRotation(t *testing.T) {
+	var c rejectionCounter
+	for i := 0; i < rejectionCounterMapCap+10; i++ {
+		c.increment("user", fmt.Sprintf("user-%d", i))
+	}
+
+	c.mu.Lock()
+	total := len(c.current) + len(c.previous)
+	currentLen := len(c.current)
+	c.mu.Unlock()
+	if total > 2*rejectionCounterMapCap {
+		t.Errorf("tracked entries = %d, want <= %d (2x cap)", total, 2*rejectionCounterMapCap)
+	}
+	if currentLen > rejectionCounterMapCap {
+		t.Errorf("current generation = %d entries, want <= %d", currentLen, rejectionCounterMapCap)
+	}
+}
+
+// TestRejectionCounter_Increment_AccumulatesPerScope proves the ordinary
+// path: repeated increments for the same (kind, id) accumulate in one
+// series rather than creating a new one each time.
+func TestRejectionCounter_Increment_AccumulatesPerScope(t *testing.T) {
+	var c rejectionCounter
+	c.increment("user", "alice")
+	c.increment("user", "alice")
+	c.increment("user", "bob")
+
+	snaps := c.snapshot()
+	got := map[rejectionScope]int64{}
+	for _, s := range snaps {
+		got[rejectionScope{kind: s.kind, id: s.id}] = s.count
+	}
+	if got[rejectionScope{kind: "user", id: "alice"}] != 2 {
+		t.Errorf("alice count = %d, want 2", got[rejectionScope{kind: "user", id: "alice"}])
+	}
+	if got[rejectionScope{kind: "user", id: "bob"}] != 1 {
+		t.Errorf("bob count = %d, want 1", got[rejectionScope{kind: "user", id: "bob"}])
+	}
+}
+
+// TestLimiter_PruneRejections_RemovesDeletedUserScope is the mutation-
+// proofing test for pruneRejections/pruneUserScopes: a user's rejection
+// series must disappear once that user is no longer in keep (the shape
+// a real file-sourced hot-reload produces, via Gateway.
+// pruneRejectionsAfterReload, metrics.go), while an unrelated user's
+// series, and every non-"user" scope kind (group, the synthetic total
+// and store_down entries), must survive untouched.
+func TestLimiter_PruneRejections_RemovesDeletedUserScope(t *testing.T) {
+	l := newLimiter(nil, true)
+	l.rejections.increment("user", "alice")
+	l.rejections.increment("user", "bob")
+	l.rejections.increment("group", "agroup")
+	l.rejections.increment(totalScopeKind, totalScopeID)
+	l.rejections.increment(rejectionScopeStoreDown, rejectionScopeStoreDown)
+
+	l.pruneRejections(map[string]bool{"bob": true}) // alice was removed from the users file; bob stays
+
+	byKey := map[rejectionScope]bool{}
+	for _, s := range l.rejectionSnapshot() {
+		byKey[rejectionScope{kind: s.kind, id: s.id}] = true
+	}
+	if byKey[rejectionScope{kind: "user", id: "alice"}] {
+		t.Error("alice's user-scope rejection entry survived pruning, want it removed")
+	}
+	if !byKey[rejectionScope{kind: "user", id: "bob"}] {
+		t.Error("bob's user-scope rejection entry was pruned, want it kept (bob is still a known user)")
+	}
+	if !byKey[rejectionScope{kind: "group", id: "agroup"}] {
+		t.Error("group-scope entry was pruned — pruning must only ever touch \"user\"-kind scopes")
+	}
+	if !byKey[rejectionScope{kind: totalScopeKind, id: totalScopeID}] {
+		t.Error("the synthetic total-scope entry was pruned — pruning must only ever touch \"user\"-kind scopes")
+	}
+	if !byKey[rejectionScope{kind: rejectionScopeStoreDown, id: rejectionScopeStoreDown}] {
+		t.Error("the synthetic store_down entry was pruned — pruning must only ever touch \"user\"-kind scopes")
 	}
 }

llmgateway.go

@@ -830,7 +830,14 @@ func attachUsersFile(auth *authStore, path string, log gatewayLogger) error {
 func (g *Gateway) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	sw := &statusTrackingWriter{ResponseWriter: w}
 	defer recoverPanic(sw, g)
-	g.auth.maybeReload()
+	// pruneRejectionsAfterReload only runs when maybeReload just performed
+	// a REAL reload (users_file.go's own doc comment explains why it must
+	// never run on every request) — a deleted file-sourced user must not
+	// keep exporting llmgateway_rate_limit_rejections_total forever
+	// (metrics.go).
+	if g.auth.maybeReload() {
+		g.pruneRejectionsAfterReload()
+	}
 	g.registry.maybeRefresh(r.Context())
 
 	if r.Method == http.MethodGet && r.URL.Path == "/v1/models" {

users_file.go

@@ -60,9 +60,21 @@ type gatewayLogger interface {
 // replaceFileUsers rejecting the new set — it logs via a.log.errorf and
 // keeps the last good set. A successful reload that changes the
 // file-sourced user count logs via a.log.logf.
-func (a *authStore) maybeReload() {
+//
+// Returns true only when a reload actually replaced the user set (review
+// fix, adversarial verification 2026-08-23) — every early-return path
+// above (no file configured, throttled, unchanged mtime, a stat/load
+// error, or replaceFileUsers itself rejecting the new set) returns
+// false. ServeHTTP uses this to gate limiter.pruneRejections: that call
+// walks every tracked rejection scope, which is cheap only relative to a
+// real reload (itself throttled to at most once per reloadEvery and
+// gated on the file's mtime actually changing) — running it on every
+// request regardless, the way maybeReload itself must, would add real
+// per-request cost to ServeHTTP's hottest path for a reason (deleted
+// users hot-reloaded out of the file) that is rare by construction.
+func (a *authStore) maybeReload() bool {
 	if a.usersFile == nil {
-		return
+		return false
 	}
 
 	// TryLock, not Lock: ServeHTTP calls maybeReload on every request, and a
@@ -74,33 +86,33 @@ func (a *authStore) maybeReload() {
 	// throttle window) covers it. Reload is deliberately best-effort and
 	// throttled, not "every change lands on the very next request."
 	if !a.reloadMu.TryLock() {
-		return
+		return false
 	}
 	defer a.reloadMu.Unlock()
 
 	now := a.nowFn()
 	if now.Sub(a.lastCheck) < reloadEvery {
-		return
+		return false
 	}
 	a.lastCheck = now
 
 	info, err := os.Stat(a.usersFile.path)
 	if err != nil {
 		a.log.errorf("llmgateway: cannot stat users file %q: %v", a.usersFile.path, err)
-		return
+		return false
 	}
 	// Equal, not After: an After-only check misses a backward mtime move —
 	// a restore from backup, or a `cp -p` from an older file. Any
 	// difference from the last observed mtime, forward or backward, must
 	// trigger a reload; only an unchanged mtime skips it.
 	if info.ModTime().Equal(a.lastModTime) {
-		return
+		return false
 	}
 
 	users, err := a.usersFile.load()
 	if err != nil {
 		a.log.errorf("llmgateway: users file %q reload failed: %v", a.usersFile.path, err)
-		return
+		return false
 	}
 
 	a.mu.RLock()
@@ -109,11 +121,12 @@ func (a *authStore) maybeReload() {
 
 	if err := a.replaceFileUsers(users); err != nil {
 		a.log.errorf("llmgateway: users file %q reload rejected: %v", a.usersFile.path, err)
-		return
+		return false
 	}
 	a.lastModTime = info.ModTime()
 
 	if len(users) != prevCount {
 		a.log.logf("llmgateway: users file %q reload: user count changed from %d to %d", a.usersFile.path, prevCount, len(users))
 	}
+	return true
 }