diff --git a/limits.go b/limits.go index becdc2a..61ae8c9 100644 --- a/limits.go +++ b/limits.go @@ -32,7 +32,7 @@ const storeDownLatchFor = 5 * time.Second // Window identifiers used throughout windowKey, counter metrics, and // retry-after calculation. windowHour is stats-only (v0.2 data-layer // task, see hourWindowTTL): checkAndCount/account both write it, but no -// LimitsConfig field ever names it, so evaluateScope has nothing to +// LimitsConfig field ever names it, so checkAndCount has nothing to // evaluate it against — GET /admin/api/usage/history (admin.go) is its // only reader. const ( @@ -48,7 +48,8 @@ const ( // usage.prompt under metricTokIn and usage.completion under metricTokOut // separately, so the admin usage/history APIs can chart each direction on // its own. A TokensPerDay/TokensPerMonth limit still enforces a TOTAL -// budget across both — see tokenBudgetViolation. +// budget across both — see checkAndCount's own probe loop and +// buildBudgetProbes' tokens field, below. const ( metricReq = "req" metricTokIn = "tokin" @@ -495,8 +496,8 @@ type limitScope struct { // totalScopeKind and totalScopeID name the synthetic "all LLM traffic" // scope withTotalScope (routes_unified.go) appends to every metered // route's scopes slice (v0.2 data-layer task): {kind: totalScopeKind, id: -// totalScopeID}, limits always nil. evaluateScope's nil-limits check -// (below) skips it during evaluation, so checkAndCount still counts it +// totalScopeID}, limits always nil. checkAndCount's own nil-limits check +// (below) skips it during evaluation, but still counts it // like any other scope — its req/tokin/tokout/cost counters accumulate // every LLM request across all users and groups combined, the admin // usage-history "total" series — but no configuration can ever throttle @@ -885,8 +886,8 @@ func (l *limiter) failPolicyIncrAndGetMulti(entries []counterIncr, reads []strin return incrVals, readVals, true } -// storeDownViolation is the violation checkAndCount/budgetViolation return -// when the configured store is unreachable and failOpen is false: the +// storeDownViolation is the violation checkAndCount returns when the +// configured store is unreachable and failOpen is false: the // request is refused instead of silently enforcing limits against a // non-shared fallback, so a backend outage cannot let every configured // limit go unenforced across a fleet of gateway instances. @@ -974,8 +975,8 @@ func (l *limiter) getCounter(kind, id, metric, window string, t time.Time) (int6 // checkAndCountKeysPerScope is the number of counterIncr entries // checkAndCount builds per scope (req:min, req:day, req:hour), and the -// stride its flat storeIncrMulti result is sliced back into per-scope -// counts by. +// stride storeIncrAndGetMulti's flat incrVals result is sliced back into +// per-scope counts by. const checkAndCountKeysPerScope = 3 // budgetProbe names one token/cost budget checkAndCount's fused admission @@ -1205,8 +1206,8 @@ func (l *limiter) account(scopes []limitScope, u usage, costMicros int64) { // // This is deliberately NOT folded into checkAndCount: a target scope // carries no limits by design (this round adds no config surface for -// MCP/agent limits), so there is nothing for evaluateScope to check and no -// violation this call could ever produce — it is pure accounting, exactly +// MCP/agent limits), so there is nothing for checkAndCount to evaluate +// and no violation this call could ever produce — it is pure accounting, exactly // like account() itself, hence the identical "no error return, ok // discarded" contract. Month exists here, uniquely, because the admin // dashboard's targets endpoint (admin.go's adminTargetCountersView) @@ -1655,7 +1656,8 @@ const usageKeysPerScope = 8 // tokensPerDay/tokensPerMonth pair (v0.2 data-layer task, tokens-in/ // tokens-out split): a caller wanting the combined total a // LimitsConfig.TokensPerDay/TokensPerMonth limit is actually evaluated -// against (tokenBudgetViolation) sums the two itself. +// against (checkAndCount's own probe loop, via buildBudgetProbes) sums +// the two itself. type scopeUsage struct { kind string // "user", "group", or "total", mirroring limitScope.kind id string diff --git a/limits_test.go b/limits_test.go index d436967..5362e11 100644 --- a/limits_test.go +++ b/limits_test.go @@ -1320,9 +1320,15 @@ func TestTokenBudget_ExactBoundaryViolates(t *testing.T) { // within one scope, requests-per-minute, then requests-per-day, then // tokens-per-day, then tokens-per-month, then cost-per-day, then // cost-per-month; across scopes, the scope earlier in the given slice -// (buildLimitScopes puts a user before their group) wins. Every subtest -// engineers TWO simultaneously-true violation conditions and asserts only -// the higher-precedence one is ever reported. +// (buildLimitScopes puts a user before their group) wins. The first six +// subtests each engineer TWO simultaneously-true violation conditions +// (one per limit type in a precedence pair) and assert only the +// higher-precedence one is ever reported; the seventh engineers only ONE +// condition (the group violates, the user carries no limits at all) to +// pin the separate claim that a later scope still gets evaluated at all +// when an earlier one has nothing to check (review fix, 2026-08-22, round +// 2: this doc comment previously claimed all seven subtests engineered +// two conditions, which was not true of the seventh). func TestCheckAndCount_FusedViolationPrecedence(t *testing.T) { t.Run("requests-per-minute beats requests-per-day in the same scope", func(t *testing.T) { l := newLimiter(nil, true) diff --git a/redis_store.go b/redis_store.go index 3ab9887..3ccf796 100644 --- a/redis_store.go +++ b/redis_store.go @@ -134,6 +134,10 @@ func (s *redisStore) incrMulti(entries []counterIncr) ([]int64, error) { // (see its own doc comment) apply to the INCRBY half of this call; the GET // half is read-only and carries no such caveat. func (s *redisStore) incrAndGetMulti(entries []counterIncr, reads []string) ([]int64, []int64, error) { + if len(entries) == 0 && len(reads) == 0 { + return nil, nil, nil + } + cmds := make([][]string, 0, len(entries)*2+len(reads)) for _, e := range entries { cmds = append(cmds, diff --git a/redis_store_test.go b/redis_store_test.go index 083f796..20d1bb3 100644 --- a/redis_store_test.go +++ b/redis_store_test.go @@ -133,6 +133,73 @@ func TestRedisStore_IncrAndGetMulti_EmptyReads(t *testing.T) { } } +// TestRedisStore_IncrAndGetMulti_NoEntriesOrReads_SkipsThePipelineCall +// asserts the empty-input guard (review fix, 2026-08-22, round 2 — +// symmetry with incrMulti's own len(entries)==0 guard): no commands are +// sent, and the call returns cleanly, when both entries and reads are +// empty. +func TestRedisStore_IncrAndGetMulti_NoEntriesOrReads_SkipsThePipelineCall(t *testing.T) { + ln := newFakeListener(t) + // No respStep script at all: any command reaching the fake server + // (including the connection-setup SELECT) would fail this test by + // timing out waiting for a script step that does not exist. + store := newRedisStore(newRESPClient(ln.Addr().String(), "", 0)) + incrVals, readVals, err := store.incrAndGetMulti(nil, nil) + if err != nil { + t.Fatalf("incrAndGetMulti: %v", err) + } + if len(incrVals) != 0 || len(readVals) != 0 { + t.Errorf("incrVals=%v readVals=%v, want both empty", incrVals, readVals) + } +} + +// TestRedisStore_IncrAndGetMulti_GetReplyErrors is the GET-reply-error +// half of the perf-review round-3 pipelining test (review fix, 2026-08-22, +// round 2 — parity with respClient.getBatch's own equivalent table, +// TestRESPClient_GetBatch, resp_test.go): a RESP error reply or a +// non-integer value on any read key fails the WHOLE call, matching +// getBatch's own all-or-nothing contract, since incrAndGetMulti decodes +// GET replies itself rather than delegating to getBatch. +func TestRedisStore_IncrAndGetMulti_GetReplyErrors(t *testing.T) { + cases := []struct { + name string + wantErrSubstr string + steps []respStep + }{ + { + name: "a RESP error reply on a read key fails the whole call", + steps: []respStep{ + {wantArgs: []string{"INCRBY", "k", "1"}, reply: []byte(":1\r\n")}, + {wantArgs: []string{"EXPIRE", "k", "60"}, reply: []byte(":1\r\n")}, + {wantArgs: []string{"GET", "budget"}, reply: []byte("-ERR busy\r\n")}, + }, + wantErrSubstr: "ERR busy", + }, + { + name: "a non-integer value on a read key fails the whole call", + steps: []respStep{ + {wantArgs: []string{"INCRBY", "k", "1"}, reply: []byte(":1\r\n")}, + {wantArgs: []string{"EXPIRE", "k", "60"}, reply: []byte(":1\r\n")}, + {wantArgs: []string{"GET", "budget"}, reply: []byte("$3\r\nabc\r\n")}, + }, + wantErrSubstr: "non-integer value", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ln := newFakeListener(t) + steps := append([]respStep{{wantArgs: []string{"SELECT", "0"}, reply: []byte("+OK\r\n")}}, c.steps...) + runFakeRESPServer(t, ln, steps) + + store := newRedisStore(newRESPClient(ln.Addr().String(), "", 0)) + entries := []counterIncr{{key: "k", delta: 1, ttl: time.Minute}} + _, _, err := store.incrAndGetMulti(entries, []string{"budget"}) + require.Error(t, err) + assert.Contains(t, err.Error(), c.wantErrSubstr) + }) + } +} + // TestRedisStore_IncrBy_RoundsSubSecondTTLUp asserts a ttl under one // second is never sent to EXPIRE as 0 (which would delete the key // immediately) — it is rounded up to a 1s floor.