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.
This commit is contained in:
2026-08-21 18:13:46 +01:00
parent 90545df588
commit 1183b6fa3c
6 changed files with 178 additions and 41 deletions
+88 -5
View File
@@ -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
+8 -7
View File
@@ -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"
+9 -8
View File
@@ -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
+7 -5
View File
@@ -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
}
+25 -12
View File
@@ -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
+41 -4
View File
@@ -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}