diff --git a/admin.go b/admin.go index d3bcea6..0049d89 100644 --- a/admin.go +++ b/admin.go @@ -308,14 +308,18 @@ type adminProviderView struct { // adminModelRateView is one upstream model's current-window attempt/ // failure counters within its provider — adminProviderView.ModelRates' -// value type (Feature A, v0.22). Field shape and meaning are identical to -// adminProviderView's own Attempts*/Failures* fields, just scoped to one -// (provider, model) pair instead of the whole provider. +// value type (Feature A, v0.22). Day window only, no AttemptsMinute/ +// FailuresMinute (SHOULD-2, v0.22 review round — removed, not merely +// left unpopulated): a dashboard with N configured models was paying N +// wasted minute-window store reads on every 5s poll for a figure that +// only ever fed a badge's title text, never its own displayed tier — see +// limits.go's providerCounterKeys/providerScopeKeyCount for the read-side +// half of this same ruling. The Providers tab's per-model badge falls +// back to a day-window-only title when these are absent; see webui's +// lib/provider-rate.ts (dayRateTitle) and ProviderRateBadge.vue. type adminModelRateView struct { - AttemptsDay int64 `json:"attemptsDay"` - FailuresDay int64 `json:"failuresDay"` - AttemptsMinute int64 `json:"attemptsMinute"` - FailuresMinute int64 `json:"failuresMinute"` + AttemptsDay int64 `json:"attemptsDay"` + FailuresDay int64 `json:"failuresDay"` } // adminRedisView is the redis status line in GET /admin/api/overview. @@ -423,10 +427,8 @@ func (g *Gateway) buildAdminOverview() adminOverviewResponse { mc := modelCounters[mi] mi++ modelRates[model] = adminModelRateView{ - AttemptsDay: mc.attemptsDay, - FailuresDay: mc.failuresDay, - AttemptsMinute: mc.attemptsMinute, - FailuresMinute: mc.failuresMinute, + AttemptsDay: mc.attemptsDay, + FailuresDay: mc.failuresDay, } } providers[i] = adminProviderView{ diff --git a/admin_test.go b/admin_test.go index 39fb3e8..9b2de92 100644 --- a/admin_test.go +++ b/admin_test.go @@ -54,6 +54,14 @@ func newAdminGatewayHandle(t *testing.T, cfg *Config) (http.Handler, *Gateway) { if !ok { t.Fatal("handler is not *Gateway") } + // SHOULD-5 (v0.22 review round): production spawns recordProviderAttempt's + // store write in its own goroutine, off a request's TTFB path — every + // admin_test.go test that asserts on a resulting provider counter (GET + // /admin/api/overview's Attempts*/Failures* fields) needs it to have + // already landed, so spawn runs synchronously here rather than per-test + // (the deterministic-test half of that dependency-injection field; see + // limiter.spawn's own doc comment, limits.go). + gw.limiter.spawn = func(f func()) { f() } return h, gw } diff --git a/limits.go b/limits.go index 6e9f63d..2bff4c7 100644 --- a/limits.go +++ b/limits.go @@ -1,6 +1,7 @@ package traefikllmgateway import ( + "context" "fmt" "math" "net/http" @@ -483,9 +484,11 @@ type limitViolation struct { // limiter enforces per-minute/day/month request, token, and cost limits // using fixed windows keyed by windowKey. type limiter struct { - store counterStore // configured backend; nil means always use fallback (see newLimiter) - fallback *memoryStore // in-process counter store, always available - nowFn func() time.Time // injected for tests; defaults to time.Now + lastLogAt time.Time // guarded by logMu; last time a store error was logged + lastStoreFailure time.Time // guarded by logMu; zero means the store-down latch is not open (see storeLatched) + store counterStore // configured backend; nil means always use fallback (see newLimiter) + fallback *memoryStore // in-process counter store, always available + nowFn func() time.Time // injected for tests; defaults to time.Now // logf is a bound method value (g.errorf), injectable for tests; defaults // to a no-op. Its own call site (logStoreError) passes exactly one // variadic argument — never extend that to two or more without first @@ -493,9 +496,24 @@ type limiter struct { // field of variadic func type crashes Yaegi v0.16.1's CFG builder past // one variadic argument, even though the identical call through a // method or interface method does not. - logf func(format string, args ...any) - lastLogAt time.Time // guarded by logMu; last time a store error was logged - lastStoreFailure time.Time // guarded by logMu; zero means the store-down latch is not open (see storeLatched) + logf func(format string, args ...any) + // spawn runs f, by default in its own goroutine — the SHOULD-5 ruling + // (v0.22 review round): recordProviderAttempt's store write is + // telemetry-only, and must never sit on a streaming response's + // time-to-first-byte path (a synchronous storeIncrMulti round trip + // there would delay every first chunk by however long that write + // takes). newLimiter always wires a production limiter's spawn to a + // real `go f()`; a test that needs recordProviderAttempt's write to + // have already landed by the time it reads a counter back overrides + // spawn to run f synchronously instead — the same dependency- + // injection idiom nowFn already uses on this struct, and waitFn uses + // on retryPolicy (retry.go). A goroutine spawned this way outlives + // the request that triggered it by design: a write still in flight at + // process shutdown is lost. Accepted — this is telemetry, nothing in + // the request path gates on it, and the fixed-window counters it + // writes to are inherently approximate already (a counter reset at a + // bucket boundary loses whatever was in the previous bucket too). + spawn func(func()) // lastErrMsg is the message of the most recent store operation // failure, guarded by logMu alongside lastStoreFailure. It is never // cleared on a later success — "last store error" for the admin @@ -521,6 +539,7 @@ func newLimiter(store counterStore, failOpen bool) *limiter { nowFn: time.Now, logf: func(string, ...any) {}, failOpen: failOpen, + spawn: func(f func()) { go f() }, } } @@ -1171,10 +1190,75 @@ const ( // pricing.go's unifiedCostMicros and the admin API already use elsewhere // for a (provider, model) pair, so a reader who already knows that // convention needs no new one here. +// +// The "/" join is unambiguous even though model itself may legitimately +// contain "/" (a discovered id like "uni/deepseek-v4-flash-0731" — +// routableModelId's own doc comment, webui/src/lib/format.ts, documents +// the identical case for the webui's copy of this convention): provider +// is always validated against configNamePattern (providers.go) before it +// can reach here, and that pattern's character class excludes "/" +// entirely. Two distinct (provider, model) pairs can therefore never +// produce the identical joined string — a provider name can never itself +// contain the separator a model id might. func providerModelScopeID(provider, model string) string { return provider + "/" + model } +// sentinelUnwrapper is the errors.Unwrap interface, matched bare (a comma-ok +// type assertion, never errors.Is/errors.As) — see matchesSentinel's own +// doc comment for why. +type sentinelUnwrapper interface { + Unwrap() error +} + +// matchesSentinel reports whether err IS, or wraps, sentinel — the exact +// semantics errors.Is applies for a plain sentinel comparison (neither +// context.DeadlineExceeded nor context.Canceled defines an Is(error) +// method, so there is no behavioral difference to preserve), implemented +// by hand-walking err's Unwrap chain instead of calling errors.Is itself. +// +// This is deliberate, not a style preference: providerHTTPError's own doc +// comment (providers.go) already documents that errors.As panics under +// Yaegi when checking whether an interpreted pointer type implements +// error, which is why handleAdapterError uses a bare type assertion +// instead. errors.Is walks the identical reflect-based machinery +// internally (both live in package errors and share its Is/As helper +// code) and carries the same risk — recordProviderAttempt is exercised +// under Yaegi too (tools/yaegi-check's harness drives a real chat +// completion through it), so this sidesteps the trap entirely: a bare +// `err.(sentinelUnwrapper)` comma-ok assertion is plain interface +// satisfaction, never reflection over a possibly-interpreted concrete +// type. +func matchesSentinel(err, sentinel error) bool { + for err != nil { + if err == sentinel { //nolint:errorlint // deliberate identity comparison; see doc comment above + return true + } + u, ok := err.(sentinelUnwrapper) + if !ok { + return false + } + err = u.Unwrap() + } + return false +} + +// isDeadlineExceeded reports whether err IS, or wraps, context. +// DeadlineExceeded — SHOULD-1 ruling (v0.22 review round): retry.go's +// isTransient deliberately excludes context.DeadlineExceeded from what it +// retries (retrying after the gateway's own deadline already lapsed is +// pointless), and recordProviderAttempt's fail determination below reuses +// isTransient unforked for every other classification — but a deadline +// the GATEWAY set and the upstream never answered inside is very much a +// provider-health signal, arguably the clearest one isTransient's +// retry-specific exclusion was never meant to hide from accounting. A +// context.Canceled error (the client walked away) is deliberately NOT +// matched here — that is not the provider's fault, and stays an +// attempt-only outcome, exactly like isTransient already treats it. +func isDeadlineExceeded(err error) bool { + return matchesSentinel(err, context.DeadlineExceeded) +} + // recordProviderAttempt accounts ONE upstream HTTP attempt against // provider (Feature A, v0.22), and against the (provider, model) pair too // when model is non-empty — a caller that cannot cheaply know the upstream @@ -1183,13 +1267,19 @@ func providerModelScopeID(provider, model string) string { // body, read only after the attempt already happened) passes "" and gets // provider-level accounting only. // -// Every attempt increments an "attempts" counter; only a provider-fault -// outcome — exactly isTransient(resp, err)'s own classification -// (retry.go: a transport/connection error, HTTP 429, or a 5xx status) — -// additionally increments a "failures" counter. Deliberately reused rather -// than forked: a 4xx-but-not-429 response means the provider answered -// correctly to a request it did not like, which is not a provider-health -// signal, exactly the line isTransient already draws for "worth retrying". +// Every attempt increments an "attempts" counter; a provider-fault +// outcome additionally increments a "failures" counter. That outcome is +// exactly isTransient(resp, err)'s own classification (retry.go: a +// transport/connection error, HTTP 429, or a 5xx status) — reused, not +// forked, per spec ruling — WITH ONE NARROW ADDITION (SHOULD-1, v0.22 +// review round): isDeadlineExceeded(err) also counts as a failure, even +// though isTransient itself returns false for it (see isDeadlineExceeded's +// own doc comment for why the two functions correctly disagree here). A +// context.Canceled error still counts as an attempt only, from both +// functions alike — the client walked away, not the provider's fault. A +// 4xx-but-not-429 response means the provider answered correctly to a +// request it did not like, which is not a provider-health signal, exactly +// the line isTransient already draws for "worth retrying". // // attempt-accounting: this is invoked once per upstream attempt, not once // per logical request — attemptRecorderFromContext (providers.go) is @@ -1200,16 +1290,28 @@ func providerModelScopeID(provider, model string) string { // countTargetRequests/-Request already apply to target scopes (mcp_a2a.go // callers), consistent with those counters' own attempt/request semantics. // -// Both counters are written at minute AND day granularity only — no month -// window: provider health is a now-and-today question (the Providers tab's -// success-rate badge, webui), not a billing one, so there is nothing here -// for a month-long retention window to serve. +// Both counters are written at minute AND day granularity, for both the +// provider scope and the (provider, model) scope — the write side is +// unaffected by SHOULD-2's read-side change to providerUsage/ +// providerCounterKeys below: writing both windows costs nothing extra +// (already one batched storeIncrMulti call either way), and it leaves +// the model-level minute counters available in the store for a future +// caller even though GET /admin/api/overview no longer reads them today. +// No month window: provider health is a now-and-today question (the +// Providers tab's success-rate badge, webui), not a billing one, so there +// is nothing here for a month-long retention window to serve. +// +// The store write itself is fire-and-forget (l.spawn — SHOULD-5, v0.22 +// review round): entries is finished being built before spawn is called, +// and never touched again afterward, so capturing it in the closure below +// is race-free even though the write itself now runs on a goroutine this +// function does not wait for. func (l *limiter) recordProviderAttempt(provider, model string, resp *http.Response, err error) { if provider == "" { return } now := l.now() - fail := isTransient(resp, err) + fail := isTransient(resp, err) || isDeadlineExceeded(err) entries := make([]counterIncr, 0, 8) entries = append(entries, @@ -1235,22 +1337,46 @@ func (l *limiter) recordProviderAttempt(provider, model string, resp *http.Respo ) } } - l.storeIncrMulti(entries) // no error return by contract (doc comment above, mirroring account()); ok is intentionally discarded + l.spawn(func() { + l.storeIncrMulti(entries) // no error return by contract (doc comment above, mirroring account()); ok is intentionally discarded + }) } // providerCounterKeysPerScope is the number of windowKey strings -// providerCounterKeys builds per scope, and the stride providerUsage's -// flat storeGetMulti result is sliced back into per-scope chunks by — -// mirrors targetCounterKeysPerScope's own convention for the identical -// reason. -const providerCounterKeysPerScope = 4 +// providerCounterKeys builds for a PROVIDER-level (kindProvider, or any +// kind other than kindProviderModel) scope — attempt/fail at minute, then +// attempt/fail at day: 4. A kindProviderModel scope reads half as many +// (providerModelCounterKeysPerScope, below) — SHOULD-2 ruling (v0.22 +// review round): GET /admin/api/overview's own per-model minute counters +// fed nothing but a badge title, so a dashboard with N models paid N +// wasted minute-window reads on every 5s poll for data nobody displayed. +// providerCounterKeys and providerUsage's own slicing loop each branch on +// sc.kind == kindProviderModel directly rather than through a shared +// "key count for this kind" helper — with exactly two kinds and two +// branches, a third layer of indirection bought nothing a direct +// comparison did not already say more plainly, mirroring +// targetCounterKeysPerScope's fixed-stride convention exactly where the +// stride really IS fixed (every target scope there reads the same three +// keys) and diverging from it only where it now is not. +const ( + providerCounterKeysPerScope = 4 + providerModelCounterKeysPerScope = 2 +) -// providerCounterKeys returns the providerCounterKeysPerScope windowKey -// strings providerUsage reads for one (kind, id) scope at time now — -// attempt/fail at minute, then attempt/fail at day, matching -// providerCounters' own field order exactly so providerUsage can map -// storeGetMulti's result slice back by plain index. +// providerCounterKeys returns providerCounterKeysPerScope (or, for a +// kindProviderModel scope, providerModelCounterKeysPerScope) windowKey +// strings providerUsage reads for one (kind, id) scope at time now: a +// kindProviderModel scope gets attempt/fail at day only; every other kind +// (kindProvider) gets attempt/fail at minute, then attempt/fail at day — +// matching providerCounters' own field order exactly so providerUsage can +// map storeGetMulti's result slice back by plain index. func providerCounterKeys(kind, id string, now time.Time) []string { + if kind == kindProviderModel { + return []string{ + windowKey(kind, id, metricProvAttempt, windowDay, now), + windowKey(kind, id, metricProvFail, windowDay, now), + } + } return []string{ windowKey(kind, id, metricProvAttempt, windowMin, now), windowKey(kind, id, metricProvFail, windowMin, now), @@ -1262,6 +1388,11 @@ func providerCounterKeys(kind, id string, now time.Time) []string { // providerCounters is one provider's or (provider, model) pair's current- // window attempt/failure counters (Feature A, v0.22) — mirrors // targetCounters' own shape and read-only, admin-view-only role. +// attemptsMinute/failuresMinute stay at their zero value for a +// kindProviderModel scope (SHOULD-2, providerCounterKeys' own doc +// comment): providerUsage never reads a model scope's minute keys at +// all, so this is not a real "zero traffic this minute" reading for a +// model the way it is for a provider — a caller must not treat it as one. type providerCounters struct { attemptsMinute int64 failuresMinute int64 @@ -1283,6 +1414,12 @@ type providerCounters struct { // here: nothing downstream treats a provider's zero counters as // "confirmed no traffic" the way scopeUsage.storeDown guards against for // a limited scope. +// +// Slicing is by running offset, not a fixed i*stride multiply (SHOULD-2, +// providerCounterKeysPerScope's own doc comment): a kindProvider and a +// kindProviderModel scope contribute a different number of keys to +// allKeys, so the flat result cannot be sliced back by a single constant +// stride the way targetUsage's fixed-shape scopes can. func (l *limiter) providerUsage(scopes []limitScope) []providerCounters { out := make([]providerCounters, len(scopes)) if len(scopes) == 0 { @@ -1291,7 +1428,9 @@ func (l *limiter) providerUsage(scopes []limitScope) []providerCounters { now := l.now() allKeys := make([]string, 0, len(scopes)*providerCounterKeysPerScope) - for _, sc := range scopes { + offsets := make([]int, len(scopes)) + for i, sc := range scopes { + offsets[i] = len(allKeys) allKeys = append(allKeys, providerCounterKeys(sc.kind, sc.id, now)...) } @@ -1300,9 +1439,18 @@ func (l *limiter) providerUsage(scopes []limitScope) []providerCounters { return out // zero-value counters; see doc comment above } - for i := range scopes { - v := vals[i*providerCounterKeysPerScope : i*providerCounterKeysPerScope+providerCounterKeysPerScope] - out[i] = providerCounters{attemptsMinute: v[0], failuresMinute: v[1], attemptsDay: v[2], failuresDay: v[3]} + for i, sc := range scopes { + start := offsets[i] + if sc.kind == kindProviderModel { + out[i] = providerCounters{attemptsDay: vals[start], failuresDay: vals[start+1]} + continue + } + out[i] = providerCounters{ + attemptsMinute: vals[start], + failuresMinute: vals[start+1], + attemptsDay: vals[start+2], + failuresDay: vals[start+3], + } } return out } diff --git a/limits_test.go b/limits_test.go index cd8ee9f..292404a 100644 --- a/limits_test.go +++ b/limits_test.go @@ -1379,13 +1379,33 @@ func TestMemoryStore_IncrMulti_DayCounterStillClampsAt48h(t *testing.T) { // --- Feature A (v0.22): per-provider/per-(provider,model) success-rate accounting --- +// newSyncLimiter builds a limiter exactly like newLimiter, but with spawn +// overridden to run synchronously instead of in its own goroutine — every +// recordProviderAttempt test in this file needs its store write to have +// already landed by the time it reads the counter back. Production +// spawns a real goroutine there (SHOULD-5 ruling, v0.22 review round: off +// a streaming response's TTFB path); this is the deterministic-test half +// of that same dependency-injection field, matching nowFn/waitFn's +// existing pattern elsewhere in this package. +func newSyncLimiter(store counterStore, failOpen bool) *limiter { + l := newLimiter(store, failOpen) + l.spawn = func(f func()) { f() } + return l +} + // TestRecordProviderAttempt_ClassifiesUsingIsTransient proves // recordProviderAttempt's failure classification is exactly isTransient's // own — reused, not forked (spec ruling) — across the full outcome table: // success, a non-429 4xx (still a "success" for provider-health purposes: // the provider answered correctly to a request it did not like), 429, a // 5xx, a plain network error, and a context-canceled error (not a -// provider fault, per isTransient's own carve-out). +// provider fault, per isTransient's own carve-out) — PLUS the SHOULD-1 +// addition (v0.22 review round): a context.DeadlineExceeded error (a +// hung upstream the gateway's own deadline finally cut off) counts as a +// failure even though isTransient itself excludes it, both bare and +// wrapped (proving isDeadlineExceeded's manual Unwrap walk actually +// walks) — while context.Canceled, wrapped the identical way, still does +// not. func TestRecordProviderAttempt_ClassifiesUsingIsTransient(t *testing.T) { tests := []struct { err error @@ -1400,11 +1420,14 @@ func TestRecordProviderAttempt_ClassifiesUsingIsTransient(t *testing.T) { {name: "503 service unavailable", resp: &http.Response{StatusCode: http.StatusServiceUnavailable}, wantFail: true}, {name: "network error", err: errors.New("dial tcp: connection refused"), wantFail: true}, {name: "context canceled (not a provider fault)", err: context.Canceled, wantFail: false}, + {name: "context deadline exceeded (hung upstream timed out — SHOULD-1)", err: context.DeadlineExceeded, wantFail: true}, + {name: "wrapped context deadline exceeded (proves the manual Unwrap walk)", err: fmt.Errorf("%w: dial timeout", context.DeadlineExceeded), wantFail: true}, + {name: "wrapped context canceled stays non-failure too", err: fmt.Errorf("%w: client hung up", context.Canceled), wantFail: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - l := newLimiter(nil, true) + l := newSyncLimiter(nil, true) now := time.Date(2026, 8, 21, 12, 0, 0, 0, time.UTC) l.nowFn = func() time.Time { return now } @@ -1447,7 +1470,7 @@ func TestRecordProviderAttempt_ClassifiesUsingIsTransient(t *testing.T) { // model lives in the response body, read only afterward) gets // provider-level accounting only when it passes model="". func TestRecordProviderAttempt_EmptyModel_ProviderScopeOnly(t *testing.T) { - l := newLimiter(nil, true) + l := newSyncLimiter(nil, true) now := time.Date(2026, 8, 21, 12, 0, 0, 0, time.UTC) l.nowFn = func() time.Time { return now } @@ -1467,10 +1490,40 @@ func TestRecordProviderAttempt_EmptyModel_ProviderScopeOnly(t *testing.T) { // resolved provider name (should never happen in production traffic, but // must never panic) is a silent no-op. func TestRecordProviderAttempt_EmptyProvider_NoOp(t *testing.T) { - l := newLimiter(nil, true) + l := newSyncLimiter(nil, true) l.recordProviderAttempt("", "some-model", &http.Response{StatusCode: http.StatusOK}, nil) // must not panic } +// TestIsDeadlineExceeded_YaegiSafeUnwrap exercises matchesSentinel's +// hand-rolled Unwrap walk directly (SHOULD-1, v0.22 review round): a bare +// context.DeadlineExceeded, one wrapped once, one wrapped twice (proving +// the loop actually loops, not just unwraps one level), a non-matching +// error that implements Unwrap (must walk to its end and report false, +// not stop after one failed comparison), and a plain error with no +// Unwrap method at all (the comma-ok assertion's negative case). +func TestIsDeadlineExceeded_YaegiSafeUnwrap(t *testing.T) { + tests := []struct { + err error + name string + want bool + }{ + {name: "nil error", err: nil, want: false}, + {name: "bare sentinel", err: context.DeadlineExceeded, want: true}, + {name: "wrapped once", err: fmt.Errorf("upstream: %w", context.DeadlineExceeded), want: true}, + {name: "wrapped twice", err: fmt.Errorf("outer: %w", fmt.Errorf("upstream: %w", context.DeadlineExceeded)), want: true}, + {name: "unrelated sentinel (context.Canceled)", err: context.Canceled, want: false}, + {name: "unrelated wrapped error", err: fmt.Errorf("dial: %w", errors.New("connection refused")), want: false}, + {name: "plain error, no Unwrap method", err: errors.New("boom"), want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isDeadlineExceeded(tt.err); got != tt.want { + t.Errorf("isDeadlineExceeded(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + // TestProviderModelScopeID proves the "provider/model" id convention // providerUsage/recordProviderAttempt share with pricing.go's own // unifiedCostMicros and the admin API. @@ -1485,7 +1538,11 @@ func TestProviderModelScopeID(t *testing.T) { // TestLimiterHistory_OneBatchCallOldestFirst above): it must issue // exactly ONE getMulti call for both a provider-level and a // (provider, model) scope together, and slice the flat result back to the -// right scope in order. +// right scope in order — including SHOULD-2's variable stride (v0.22 +// review round): the provider scope reads its minute window too, the +// model scope does not, so a stray seeded minute-window key under the +// model scope's own id must never leak into its result — providerUsage +// never even asks the store for it. func TestProviderUsage_BatchedSingleRoundTrip(t *testing.T) { now := time.Date(2026, 8, 21, 12, 0, 0, 0, time.UTC) countStore := &historyCountingStore{values: map[string]int64{ @@ -1494,6 +1551,13 @@ func TestProviderUsage_BatchedSingleRoundTrip(t *testing.T) { windowKey(kindProvider, "openai", metricProvAttempt, windowDay, now): 30, windowKey(kindProvider, "openai", metricProvFail, windowDay, now): 2, windowKey(kindProviderModel, "openai/gpt-4o", metricProvAttempt, windowDay, now): 10, + // Seeded but must never be read: providerCounterKeys omits a + // kindProviderModel scope's minute-window keys entirely (SHOULD-2). + // A bug that started reading them again would make this key's + // value leak into got[1].attemptsMinute below, catching the + // regression even though allKeys's own length is never asserted + // directly. + windowKey(kindProviderModel, "openai/gpt-4o", metricProvAttempt, windowMin, now): 999, }} l := newLimiter(countStore, true) l.nowFn = func() time.Time { return now } @@ -1516,6 +1580,60 @@ func TestProviderUsage_BatchedSingleRoundTrip(t *testing.T) { if got[1].attemptsDay != 10 || got[1].failuresDay != 0 { t.Errorf("model counters = %+v, want attemptsDay 10, failuresDay 0", got[1]) } + if got[1].attemptsMinute != 0 || got[1].failuresMinute != 0 { + t.Errorf("model minute counters = %+v, want the zero value (never read for a model scope — SHOULD-2)", got[1]) + } +} + +// TestProviderUsage_OffsetSlicing_ScopeOrderIndependent proves the +// running-offset slicing SHOULD-2 introduced (providerUsage no longer +// assumes a single fixed stride) is correct regardless of scope order — +// a model scope (2 keys) followed by TWO provider scopes (4 keys each) +// followed by another model scope, deliberately not the "providers then +// models" order buildAdminOverview itself always builds, to prove the +// slicing logic itself does not silently depend on that convention. +func TestProviderUsage_OffsetSlicing_ScopeOrderIndependent(t *testing.T) { + now := time.Date(2026, 8, 21, 12, 0, 0, 0, time.UTC) + countStore := &historyCountingStore{values: map[string]int64{ + windowKey(kindProviderModel, "a/m1", metricProvAttempt, windowDay, now): 1, + windowKey(kindProviderModel, "a/m1", metricProvFail, windowDay, now): 0, + windowKey(kindProvider, "a", metricProvAttempt, windowMin, now): 2, + windowKey(kindProvider, "a", metricProvFail, windowMin, now): 0, + windowKey(kindProvider, "a", metricProvAttempt, windowDay, now): 20, + windowKey(kindProvider, "a", metricProvFail, windowDay, now): 1, + windowKey(kindProvider, "b", metricProvAttempt, windowMin, now): 3, + windowKey(kindProvider, "b", metricProvFail, windowMin, now): 1, + windowKey(kindProvider, "b", metricProvAttempt, windowDay, now): 30, + windowKey(kindProvider, "b", metricProvFail, windowDay, now): 2, + windowKey(kindProviderModel, "b/m2", metricProvAttempt, windowDay, now): 4, + windowKey(kindProviderModel, "b/m2", metricProvFail, windowDay, now): 4, + }} + l := newLimiter(countStore, true) + l.nowFn = func() time.Time { return now } + + scopes := []limitScope{ + {kind: kindProviderModel, id: "a/m1"}, + {kind: kindProvider, id: "a"}, + {kind: kindProvider, id: "b"}, + {kind: kindProviderModel, id: "b/m2"}, + } + got := l.providerUsage(scopes) + + if len(got) != 4 { + t.Fatalf("len(got) = %d, want 4", len(got)) + } + if got[0].attemptsDay != 1 || got[0].failuresDay != 0 { + t.Errorf("scope[0] (model a/m1) = %+v, want attemptsDay 1, failuresDay 0", got[0]) + } + if got[1].attemptsMinute != 2 || got[1].attemptsDay != 20 || got[1].failuresDay != 1 { + t.Errorf("scope[1] (provider a) = %+v, want attemptsMinute 2, attemptsDay 20, failuresDay 1", got[1]) + } + if got[2].attemptsMinute != 3 || got[2].failuresMinute != 1 || got[2].attemptsDay != 30 || got[2].failuresDay != 2 { + t.Errorf("scope[2] (provider b) = %+v, want {3 1 30 2}", got[2]) + } + if got[3].attemptsDay != 4 || got[3].failuresDay != 4 { + t.Errorf("scope[3] (model b/m2) = %+v, want attemptsDay 4, failuresDay 4", got[3]) + } } // TestProviderUsage_EmptyScopes_ReturnsEmptyWithoutTouchingStore mirrors diff --git a/routes_media_test.go b/routes_media_test.go index 4d19e89..6caa1c2 100644 --- a/routes_media_test.go +++ b/routes_media_test.go @@ -100,6 +100,14 @@ func newMediaTestGateway(t *testing.T, cfg *Config) *Gateway { if !ok { t.Fatal("handler is not *Gateway") } + // SHOULD-5 (v0.22 review round): production spawns recordProviderAttempt's + // store write in its own goroutine, off a media route's TTFB path — every + // caller of this shared helper that asserts on a resulting provider + // counter needs it to have already landed, so spawn runs synchronously + // here rather than per-test (the deterministic-test half of that + // dependency-injection field; see limiter.spawn's own doc comment, + // limits.go). + gw.limiter.spawn = func(f func()) { f() } return gw } diff --git a/routes_passthrough_test.go b/routes_passthrough_test.go index da716c9..79c6683 100644 --- a/routes_passthrough_test.go +++ b/routes_passthrough_test.go @@ -448,6 +448,13 @@ func TestHandlePassthrough_NonStreamJSON_AccountsUsage(t *testing.T) { if !ok { t.Fatal("handler is not *Gateway") } + // SHOULD-5 (v0.22 review round): production spawns recordProviderAttempt's + // store write in its own goroutine, off this route's TTFB path — this + // test asserts on the resulting counter immediately after ServeHTTP + // returns, so it overrides spawn to run synchronously instead (the + // deterministic-test half of that dependency-injection field; see + // limiter.spawn's own doc comment, limits.go). + gw.limiter.spawn = func(f func()) { f() } req := httptest.NewRequest(http.MethodPost, "/openai/v1/native-endpoint", strings.NewReader(`{}`)) req.Header.Set("Authorization", "Bearer sk-alice") @@ -524,6 +531,9 @@ func TestHandlePassthrough_DeadUpstream_RecordsProviderFailure(t *testing.T) { if !ok { t.Fatal("handler is not *Gateway") } + // SHOULD-5 (v0.22 review round): see the identical override in + // TestHandlePassthrough_NonStreamJSON_AccountsUsage above. + gw.limiter.spawn = func(f func()) { f() } req := httptest.NewRequest(http.MethodPost, "/openai/v1/native-endpoint", strings.NewReader(`{}`)) req.Header.Set("Authorization", "Bearer sk-alice")