From 1183b6fa3c041bee91ad3070a29d6374e9e91369 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 21 Aug 2026 18:13:46 +0100 Subject: [PATCH] feat(limits): unconditional usage accounting buildLimitScopes previously omitted a user or group scope entirely whenever that entity had no configured Limits, so a limit-less user/group never accumulated ANY usage counters. Root-caused live: the admin dashboard's total kept climbing while every per-user row stayed zero until an operator added phantom, unreachable requestsPerDay limits everywhere just to make counters build at all. buildLimitScopes now always builds a user and a group scope. Limits still gate enforcement only: evaluateScope already no-ops on a nil Limits field, so a limit-less scope is counted but never violates. Update stale doc comments coupling accounting to configured limits (evaluateScope, the integration alice fixture, routes_media_test.go, routes_unified_test.go, admin_test.go's admin1 assertion) and add: - TestBuildLimitScopes_AlwaysBuildsBothScopes: table-driven over every nil/non-nil user/group limits combination. - TestAdminUsage_LimitlessUserAndGroup_AccumulateRealTraffic: end-to-end regression proving a limit-less user and group accumulate real request/token counters visible via GET /admin/api/usage. --- admin_test.go | 93 ++++++++++++++++++++++++++-- integration/traefik/dynamic.yml.tmpl | 15 ++--- limits.go | 17 ++--- routes_media_test.go | 12 ++-- routes_unified.go | 37 +++++++---- routes_unified_test.go | 45 ++++++++++++-- 6 files changed, 178 insertions(+), 41 deletions(-) diff --git a/admin_test.go b/admin_test.go index 32f58f6..3aa8782 100644 --- a/admin_test.go +++ b/admin_test.go @@ -450,13 +450,18 @@ func TestAdminUsage_MathAgainstSeededCounters(t *testing.T) { } // admin1 itself must also be listed (buildAdminUsage lists every - // active user/group, not just ones with seeded traffic): admin1 has - // no user-level limits configured, so buildLimitScopes never builds a - // "user"/admin1 scope for its own requests (ruling e) and its own - // counters stay at zero — but the row must still be present. + // active user/group, not just ones with seeded traffic): this test + // never seeds any counter for admin1 (only alice/agroup/total above), + // so its row reads genuinely zero — but the row must still be present. + // This is unrelated to buildLimitScopes (routes_unified.go): since the + // v0.21 accounting fix, that function always builds a scope regardless + // of limits, and separately, handleAdminAPI itself never calls + // checkAndCount for ANY admin route caller (admin.go's own doc + // comment) — admin1's zero here is simply "no traffic was ever + // generated for it in this test". admin1 := findUsageEntry(t, got.Users, "admin1") if admin1.RequestsPerMinute != 0 { - t.Errorf("admin1 requestsPerMinute = %d, want 0 (admin1 has no user-level limits, so its own requests never build a user scope)", admin1.RequestsPerMinute) + t.Errorf("admin1 requestsPerMinute = %d, want 0 (no traffic was seeded for admin1 in this test)", admin1.RequestsPerMinute) } if admin1.StoreDown { t.Error("admin1.storeDown must be false") @@ -496,6 +501,84 @@ func findUsageEntry(t *testing.T, entries []adminUsageEntryView, id string) admi return adminUsageEntryView{} } +// TestAdminUsage_LimitlessUserAndGroup_AccumulateRealTraffic is the +// regression test for the production accounting bug (v0.21, root-caused +// live): buildLimitScopes (routes_unified.go) used to build a counter scope +// for a user or group ONLY when that entity had its own Limits configured, +// so a limit-less user/group never accumulated usage at all — the +// dashboard's total kept climbing while every such row stayed zero, until +// an operator worked around it by adding phantom, deliberately-unreachable +// requestsPerDay limits everywhere. "nolim" and "nolimgroup" here carry no +// Limits whatsoever (both nil, never set), yet after one real, end-to-end +// chat-completion request — driven through the actual unified route, not a +// direct counter seed — their rows in GET /admin/api/usage must already +// show real, non-zero request and token counters. +func TestAdminUsage_LimitlessUserAndGroup_AccumulateRealTraffic(t *testing.T) { + t.Parallel() + const respBody = `{"id":"c1","object":"chat.completion","model":"gpt-test","choices":[{"index":0,"message":{"role":"assistant","content":"hi"},"finish_reason":"stop"}],"usage":{"prompt_tokens":7,"completion_tokens":3}}` + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(respBody)) + })) + defer srv.Close() + + cfg := CreateConfig() + cfg.Admin = &AdminConfig{Enabled: true} + cfg.Providers = map[string]*ProviderConfig{ + "openai": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up", Models: []string{"gpt-test"}}, + } + cfg.Groups = map[string]*GroupConfig{ + "nolimgroup": {}, // no Limits configured + "admingroup": {}, + } + cfg.Users = &UsersConfig{Inline: []*UserConfig{ + {Name: "nolim", Group: "nolimgroup", APIKey: "sk-nolim"}, // no Limits configured + {Name: "admin1", Group: "admingroup", APIKey: "sk-admin1", Admin: true}, + }} + h, _ := newAdminGatewayHandle(t, cfg) + + body := map[string]any{"model": "gpt-test", "messages": []any{map[string]any{"role": "user", "content": "hi"}}} + chatReq := newUnifiedRequest(t, http.MethodPost, "/v1/chat/completions", "sk-nolim", body) + chatRec := httptest.NewRecorder() + h.ServeHTTP(chatRec, chatReq) + if chatRec.Code != http.StatusOK { + t.Fatalf("chat completion status = %d, want 200, body=%s", chatRec.Code, chatRec.Body.String()) + } + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, adminUsagePath, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("GET /admin/api/usage status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var got adminUsageResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + + nolim := findUsageEntry(t, got.Users, "nolim") + if nolim.Limits != nil { + t.Errorf("nolim.Limits = %+v, want nil (no limits configured)", nolim.Limits) + } + if nolim.RequestsPerDay != 1 || nolim.RequestsPerMinute != 1 { + t.Errorf("nolim requests = min:%d day:%d, want 1/1 (the bug this test guards: a limit-less user never accumulated usage)", nolim.RequestsPerMinute, nolim.RequestsPerDay) + } + if nolim.TokensInPerDay != 7 || nolim.TokensOutPerDay != 3 { + t.Errorf("nolim tokens = in/day:%d out/day:%d, want 7/3", nolim.TokensInPerDay, nolim.TokensOutPerDay) + } + + nolimgroup := findUsageEntry(t, got.Groups, "nolimgroup") + if nolimgroup.Limits != nil { + t.Errorf("nolimgroup.Limits = %+v, want nil (no limits configured)", nolimgroup.Limits) + } + if nolimgroup.RequestsPerDay != 1 || nolimgroup.RequestsPerMinute != 1 { + t.Errorf("nolimgroup requests = min:%d day:%d, want 1/1 (the bug this test guards: a limit-less group never accumulated usage)", nolimgroup.RequestsPerMinute, nolimgroup.RequestsPerDay) + } + if nolimgroup.TokensInPerDay != 7 || nolimgroup.TokensOutPerDay != 3 { + t.Errorf("nolimgroup tokens = in/day:%d out/day:%d, want 7/3", nolimgroup.TokensInPerDay, nolimgroup.TokensOutPerDay) + } +} + // accessListKeys are adminUsageEntryView's four group-access JSON keys // (admin.go) — the set TestAdminUsage_GroupAccessLists checks both at the // typed-struct level and, since a nil Go slice and an absent JSON key diff --git a/integration/traefik/dynamic.yml.tmpl b/integration/traefik/dynamic.yml.tmpl index 6aaa836..5d2f881 100644 --- a/integration/traefik/dynamic.yml.tmpl +++ b/integration/traefik/dynamic.yml.tmpl @@ -83,13 +83,14 @@ http: users: inline: # limits.requestsPerDay is set (generously — every test in - # this suite together never approaches it) purely so - # buildLimitScopes (routes_unified.go/routes_media.go/ - # admin.go) builds a "user" counter scope for alice at all: - # a nil per-user Limits means only the group's own counters - # ever increment, which would make the admin usage test's - # "alice's own request counters are > 0" assertion always - # read zero regardless of how much traffic she generated. + # this suite together never approaches it) to exercise real + # per-user ENFORCEMENT alongside accounting, not because it is + # required for alice's own counters to move at all: since the + # v0.21 accounting fix, buildLimitScopes (routes_unified.go/ + # routes_media.go/admin.go) always builds a "user" counter + # scope, limits or not, so the admin usage test's "alice's own + # request counters are > 0" assertion would hold even with + # this field unset. - name: alice group: eng apiKey: "sk-int-alice" diff --git a/limits.go b/limits.go index fbf051d..8a477c1 100644 --- a/limits.go +++ b/limits.go @@ -860,14 +860,15 @@ func (l *limiter) checkAndCount(scopes []limitScope) *limitViolation { // evaluateScope checks one scope's already-set limits against its // just-incremented request counts and its accumulated token/cost counts. -// A nil limits field means nothing to evaluate for this scope. In -// principle a nil-limits scope can still reach here with its req counters -// already incremented by checkAndCount, since that increment runs before -// any evaluation; in practice the unified route's buildLimitScopes -// (routes_unified.go, ruling e) never builds one — it omits a user or -// group scope from the slice entirely whenever that scope's own limits -// are nil — so this check is a defensive no-op today, not a path any -// current caller exercises. +// A nil limits field means nothing to evaluate for this scope — the real, +// commonly-exercised path since the v0.21 accounting fix: buildLimitScopes +// (routes_unified.go) now ALWAYS builds a user and a group scope, so a +// limit-less user or group reaches here on every request, with its req +// counters already incremented by checkAndCount (that increment runs before +// any evaluation) and nothing for this function to enforce against them. +// This is exactly the mechanism that decouples accounting from +// enforcement: the scope is counted unconditionally, evaluated only when it +// actually has a limit configured. func (l *limiter) evaluateScope(sc limitScope, minCount, dayCount int64, now time.Time) *limitViolation { if sc.limits == nil { return nil diff --git a/routes_media_test.go b/routes_media_test.go index 8677a7c..23a81a8 100644 --- a/routes_media_test.go +++ b/routes_media_test.go @@ -76,11 +76,13 @@ func newMediaTestConfig(openaiURL string, models ...string) *Config { "openai": {Type: "openai", BaseURL: openaiURL, APIKey: "sk-up", Models: models}, } cfg.Groups = map[string]*GroupConfig{"default": {}} - // Limits is a non-nil (if all-zero, i.e. unlimited) *LimitsConfig so - // buildLimitScopes (routes_media.go/routes_unified.go) includes a - // "user" scope for alice — matching routes_unified_test.go's own - // happy-path fixture — otherwise checkAndCount has no scope to - // increment a counter for at all. + // Limits is a non-nil (if all-zero, i.e. unlimited) *LimitsConfig — + // matching routes_unified_test.go's own happy-path fixture. Since the + // v0.21 accounting fix, buildLimitScopes (routes_media.go/ + // routes_unified.go) always builds a "user" scope for alice regardless + // of whether Limits is set at all; this fixture keeps a non-nil value + // only for parity with the other unified-route fixtures, not because it + // is required for checkAndCount to have a scope to increment. cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice", Limits: &LimitsConfig{}}}} return cfg } diff --git a/routes_unified.go b/routes_unified.go index 95b8b38..bd68374 100644 --- a/routes_unified.go +++ b/routes_unified.go @@ -220,11 +220,28 @@ func (g *Gateway) runUnified(w http.ResponseWriter, r *http.Request, u *user, gr } // buildLimitScopes returns the limitScope slice runUnified passes to the -// limiter: a user scope only when u has its own limits configured, then a -// group scope only when grp does (ruling e) — either, both, or neither may -// apply to a given request. The user scope is listed first, so -// checkAndCount reports a user's own violation ahead of their group's when -// both are breached by the same request. +// limiter: a user scope and a group scope, ALWAYS both, unconditionally +// (v0.21 fix — see this function's doc comment history below for the bug +// this closes). The user scope is listed first, so checkAndCount reports a +// user's own violation ahead of their group's when both are breached by the +// same request. +// +// u.limits or grp.limits may be nil — that scope simply carries no limit for +// evaluateScope (limits.go) to enforce, and evaluateScope's own nil-limits +// check skips it during evaluation. It is NOT omitted from the slice: usage +// accounting (checkAndCount's own counting half, and account) is +// unconditional and must never be coupled to whether a limit happens to be +// configured. Production bug (root-caused live, 2026-08-21): the previous +// version of this function omitted a user or group scope entirely whenever +// that entity had no configured Limits, which meant a limit-less user or +// group never accumulated ANY usage counters at all — the admin dashboard's +// per-user rows all read zero while the total kept climbing, until an +// operator worked around it by adding a phantom, deliberately-unreachable +// requestsPerDay limit to every user/group just to make buildLimitScopes +// build a counter scope for them. That workaround is no longer needed: +// accounting and enforcement are now fully decoupled, matching +// buildAdminUsage's own "never omits an entity for having nil limits" +// contract (admin.go) that the dashboard's usage table already promised. // // Callers metering actual LLM traffic wrap this result in withTotalScope // before passing it to checkAndCount/account; handleAdminAPI (admin.go) @@ -232,14 +249,10 @@ func (g *Gateway) runUnified(w http.ResponseWriter, r *http.Request, u *user, gr // req/min-req/day accounting never contributes to the total-scope // LLM-traffic series (v0.2 data-layer task). func buildLimitScopes(u *user, grp *group) []limitScope { - scopes := make([]limitScope, 0, 2) - if u.limits != nil { - scopes = append(scopes, limitScope{limits: u.limits, kind: "user", id: u.name}) + return []limitScope{ + {limits: u.limits, kind: "user", id: u.name}, + {limits: grp.limits, kind: "group", id: grp.name}, } - if grp.limits != nil { - scopes = append(scopes, limitScope{limits: grp.limits, kind: "group", id: grp.name}) - } - return scopes } // withTotalScope returns scopes with the synthetic total scope diff --git a/routes_unified_test.go b/routes_unified_test.go index 0503fbd..be9a8f3 100644 --- a/routes_unified_test.go +++ b/routes_unified_test.go @@ -1139,10 +1139,10 @@ func (s *behavioralRedisServer) handle(args []string) []byte { // behavioralRedisServer and cfg.Cache enabled, an openai provider pointed // at srv, group "default" (its Cache override left to groupCache, nil // unless the caller sets it), and one user ("alice") with an (empty, -// unlimited) LimitsConfig — required so buildLimitScopes actually -// produces a scope for the limiter to count against; without one, -// checkAndCount/account never touch the store at all, and this test's -// counter assertions would trivially "pass" against uncounted zeros. +// unlimited) LimitsConfig — kept for parity with the other unified-route +// fixtures in this file, though since the v0.21 accounting fix +// buildLimitScopes always produces alice's own scope regardless of whether +// Limits is set at all. func newCacheTestGateway(t *testing.T, srv *httptest.Server, groupCache *bool, maxBodyBytes int) *Gateway { t.Helper() redisLn := newBehavioralRedisServer(t) @@ -1751,6 +1751,43 @@ func TestCacheCaptureWriter_ImplicitStatus_CapturesContentType(t *testing.T) { // canonical ("provider/model") first, falling back to bare — the upstream // model id alone — only when canonical has no configured price at all, in // neither overrides nor the built-in table. +// TestBuildLimitScopes_AlwaysBuildsBothScopes proves buildLimitScopes +// builds a user AND a group limitScope unconditionally (v0.21 accounting +// fix): whether either entity has Limits configured or not, both scopes +// must always be present — a nil Limits field turns off ENFORCEMENT +// (evaluateScope's own nil check, limits.go) for that scope, never +// accounting. Table-driven over every nil/non-nil combination of user and +// group limits. +func TestBuildLimitScopes_AlwaysBuildsBothScopes(t *testing.T) { + cases := []struct { + userLimits *LimitsConfig + grpLimits *LimitsConfig + name string + }{ + {name: "both nil (the exact bug scenario)"}, + {name: "user limited, group unlimited", userLimits: &LimitsConfig{RequestsPerMinute: 5}}, + {name: "user unlimited, group limited", grpLimits: &LimitsConfig{RequestsPerMinute: 5}}, + {name: "both limited", userLimits: &LimitsConfig{RequestsPerMinute: 5}, grpLimits: &LimitsConfig{RequestsPerMinute: 9}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + u := &user{name: "u1", limits: tc.userLimits} + grp := &group{name: "g1", limits: tc.grpLimits} + scopes := buildLimitScopes(u, grp) + + if len(scopes) != 2 { + t.Fatalf("len(scopes) = %d, want 2 (unconditional user+group accounting)", len(scopes)) + } + if scopes[0].kind != "user" || scopes[0].id != "u1" || scopes[0].limits != tc.userLimits { + t.Errorf("scopes[0] = %+v, want user scope for u1 with limits %+v", scopes[0], tc.userLimits) + } + if scopes[1].kind != "group" || scopes[1].id != "g1" || scopes[1].limits != tc.grpLimits { + t.Errorf("scopes[1] = %+v, want group scope for g1 with limits %+v", scopes[1], tc.grpLimits) + } + }) + } +} + func TestUnifiedCostMicros(t *testing.T) { u := usage{prompt: 1_000_000, completion: 1_000_000}