diff --git a/limits.go b/limits.go index 041e14b..25750db 100644 --- a/limits.go +++ b/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 { diff --git a/limits_test.go b/limits_test.go index 5362e11..2857285 100644 --- a/limits_test.go +++ b/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") } } diff --git a/llmgateway.go b/llmgateway.go index 93dd252..f0e6380 100644 --- a/llmgateway.go +++ b/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" { diff --git a/users_file.go b/users_file.go index 2b46731..f55deb0 100644 --- a/users_file.go +++ b/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 }