From 657480348b18a2151984cd06e87243a7b24d4e65 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 20 Aug 2026 22:46:06 +0100 Subject: [PATCH] fix: v0.2 final review wave Transcriptions Yaegi coverage via integration mock, flattened admin usage batch, hardened CSP, sanitized provider lastErr, source-archive prefix template, vendor export-ignore warning, release paths. --- .gitattributes | 11 ++++ .github/workflows/release.yml | 5 ++ .goreleaser.yaml | 7 +++ README.md | 55 +++++++++++++++--- admin.go | 99 +++++++++++++++++++++++++++------ admin_test.go | 53 +++++++++++++++--- integration/integration_test.go | 89 +++++++++++++++++++++++++++++ integration/mock/main.go | 22 ++++++++ limits.go | 78 ++++++++++++++++---------- 9 files changed, 359 insertions(+), 60 deletions(-) diff --git a/.gitattributes b/.gitattributes index 5b9572b..d6ecba7 100644 --- a/.gitattributes +++ b/.gitattributes @@ -3,6 +3,17 @@ # (.goreleaser.yaml), so these entries are the actual filter for the # release source tarball — not the archive block's own file globs, which # have no negation syntax and cannot exclude anything. +# WARNING (v0.2 final review wave, 2026-08-20): vendor/ is safe to +# export-ignore ONLY because every current go.mod dependency +# (github.com/stretchr/testify, its go.yaml.in/yaml/v3 indirect) is a +# TEST-only dependency, imported solely by *_test.go files — which are +# themselves export-ignored below, so the released tarball never contains +# code that imports them and never needs vendor/ to build. go.mod itself +# is NOT export-ignored and ships in every release tarball as-is. The +# moment any non-test .go file (production code, not *_test.go) imports a +# third-party package, this line MUST be removed: otherwise the tarball +# ships a go.mod requiring a module its own vendor/ was stripped out of, +# and the extracted source cannot build standalone. vendor/ export-ignore integration/ export-ignore tools/ export-ignore diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0be7935..e5f844a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -8,6 +8,11 @@ on: - "**.go" - "go.mod" - "go.sum" + - ".goreleaser.yaml" + - ".gitattributes" + - ".traefik.yml" + - "version.go" + - "workflow-prepare.sh" workflow_dispatch: permissions: diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 4ae4df8..5d505d9 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -20,6 +20,13 @@ source: enabled: true format: tar.gz name_template: "{{ .ProjectName }}_v{{ .Version }}_source" + # Extracting the tarball must land everything under one directory, not + # loose at the archive root, the same way GitHub's own source tarballs + # behave — without this, `tar xzf` scatters the plugin's files straight + # into the extracting directory. Verified with `goreleaser release + # --snapshot --clean`: the tarball's top-level entry is exactly + # traefik-llmgateway_v{version}/ (v0.2 final review wave, 2026-08-20). + prefix_template: "{{ .ProjectName }}_v{{ .Version }}/" checksum: name_template: "{{ .ProjectName }}_v{{ .Version }}_checksums.txt" diff --git a/README.md b/README.md index 65470ff..264bdc8 100644 --- a/README.md +++ b/README.md @@ -171,7 +171,17 @@ config accepts them as YAML, which decodes to the same JSON shape. | `cache` | `CacheConfig` | `{}` (disabled) | Opt-in Redis-backed response cache for unified non-streaming chat/embeddings — see [Caching](#caching). Omitted or `enabled: false` means no caching, byte-identical to a gateway built before this field existed. | | `admin` | `*AdminConfig` | `nil` (disabled) | Read-only admin dashboard — see [Admin](#admin). `nil` or `enabled: false` means the `/admin*` routes are not registered at all. | | `modelAliases` | `map[string]string` | `{}` | Operator-defined alias id → target model id — see [Model aliases](#model-aliases). Omitted or empty means no aliases, byte-identical to a gateway built before this field existed. | -| `passthroughUnknown` | `bool` | `false` | `false`: a request matching none of the plugin's routes gets a 404 JSON envelope. `true`: it falls through to the router's own backing service. | +| `passthroughUnknown` | `bool` | `false` | `false`: a request matching none of the plugin's routes gets a 404 JSON envelope. `true`: it falls through to the router's own backing service. **Does not cover the three media routes** — see the upgrade note below. | + +> **Upgrading from v0.1**: `POST /v1/images/generations`, `POST +> /v1/audio/speech`, and `POST /v1/audio/transcriptions` are now handled by +> the gateway itself, unconditionally — see [Image and audio +> endpoints](#image-and-audio-endpoints). They no longer fall through to +> `next` even when `passthroughUnknown: true`. A v0.1 deployment that +> served its own backend on any of these three exact paths, relying on +> passthrough to reach it, must move that backend to a different path +> before upgrading to v0.2 — the gateway now intercepts these paths first, +> every time. Provider, `mcpServers`, and `agents` map **keys** (names) must match `^[a-zA-Z0-9._-]+$` and must not be `v1`, `mcp`, or `a2a` — those names are @@ -348,10 +358,15 @@ kept — a bad edit to the file never breaks already-authenticated traffic. through the explicit `provider/model` form regardless of who won the bare id, and `GET /v1/models` lists both the bare (winner's) form and every `provider/model` form. -- Authorization always requires both `group.allowsModel` and +- **On the unified routes** (`modelRegistry.resolve`, used by + `/v1/chat/completions`, `/v1/embeddings`, and the three media + endpoints), authorization requires both `group.allowsModel` and `group.allowsProvider` for the resolved provider — a model that exists but the caller's group cannot reach returns 403, distinct from a model no - configured provider knows at all (404). + configured provider knows at all (404). Native passthrough + (`/{provider}/...`) resolves no model at all and checks + `group.allowsProvider` only — see the authorization row in [Unified vs. + passthrough](#unified-vs-passthrough). - **Discovery**: `GET {baseUrl}/v1/models` for an `openai`-type provider, `GET {baseUrl}/v1/models` for `anthropic`, `GET {baseUrl}/v1beta/models` for `gemini`. Runs once at plugin construction (bounded to 5s per @@ -489,9 +504,11 @@ predates this field keeps making exactly one upstream attempt per request. - **Scope**: every adapter upstream call — chat, embeddings, model - listing — across all three provider types. Native passthrough - (`/{provider}/...`) and the MCP/A2A proxy are raw reverse proxies and - are never retried; the client owns retry semantics there. + listing, and the three media endpoints (`images/generations`, + `audio/speech`, `audio/transcriptions`) — across all three provider + types. Native passthrough (`/{provider}/...`) and the MCP/A2A proxy are + raw reverse proxies and are never retried; the client owns retry + semantics there. - **Attempts is retries, not tries**: `attempts` counts retries performed **after** the first try. The default, `1`, allows one retry — two tries total, not one. The maximum, `3`, allows three retries — @@ -579,6 +596,7 @@ cached bodies for the same request. | | Unified (`/v1/...`) | Native passthrough (`/{provider}/...`) | |---|---|---| | Wire format | Always OpenAI-shaped in, OpenAI-shaped out. | The provider's own native format, untouched. | +| Authorization | `group.allowsModel` AND `group.allowsProvider`, both checked against the resolved provider (`modelRegistry.resolve` — see [Model routing](#model-routing)). | `group.allowsProvider` only. Passthrough resolves no model at all, so there is nothing for `allowsModel` to gate. | | Translation | `openai`-type: body forwarded verbatim (model id rewritten). `anthropic`/`gemini`: full bidirectional translation, including streaming, chunk by chunk. | None — a raw reverse proxy. | | Model alias in the response | Translated providers (`anthropic`, `gemini`) echo back the client's exact requested model string in the response's `model` field, even though the upstream call used the resolved provider model id. An `openai`-type response is a verbatim passthrough of the upstream body, so it carries whatever model id the upstream itself returned — this asymmetry is intentional, not a bug. | The upstream's own `model` field, verbatim — there is no alias to echo. | | Embeddings | `openai`-type: passthrough. `gemini`: mapped to `:embedContent`/`:batchEmbedContents`. `anthropic`: **501** — Anthropic's API has no embeddings endpoint. | Whatever the provider itself supports at that path; the gateway does not gate it. | @@ -592,6 +610,14 @@ cached bodies for the same request. `/v1/chat/completions` — auth, model routing, group authorization, limits, adapter call — minus response caching and token/cost accounting (below). +> **Upgrading from v0.1**: these three routes are now handled by the +> gateway unconditionally. They are matched and served before +> `passthroughUnknown` is ever consulted, so a request to any of them +> **never** falls through to `next` — not even with `passthroughUnknown: +> true`. If a v0.1 deployment pointed its router's backing service at one +> of these exact paths and relied on passthrough to reach it, that backend +> must move to a different path before upgrading. + | | `openai`-type | `gemini` | `anthropic` | |---|---|---|---| | `images/generations` | Native forward. | Translated to Imagen's `:predict` (below). | **501**. | @@ -674,7 +700,10 @@ request to any of them falls through to the plugin's existing gate: unauthenticated → 401, authenticated non-admin → 403, admin → serve. `overview` returns the provider list, groups, the configured model alias table (`alias`/`target` pairs, sorted by alias — see - [Model aliases](#model-aliases)), redis/cache status, and the plugin + [Model aliases](#model-aliases)), redis/cache/retry status (`retry`'s + `enabled`/`attempts`/`backoff` are the EFFECTIVE values after + [Retry](#retry)'s own defaulting, not the raw config — `attempts` and + `backoff` are both omitted when retry is disabled), and the plugin version string. `usage` returns every user's and every group's current-window counter values (req/min, req/day, tok/day, tok/month, cost/day, cost/month) alongside their configured limits. @@ -793,6 +822,18 @@ request to any of them falls through to the plugin's existing move a token counter and always record cost as 0, regardless of what a provider itself would charge for them — a real gap for an operator billing on these endpoints, not just an accounting nuance. +- **An upstream response over 32MiB (`maxResponseBytes`, `providers.go`) + is silently truncated, not rejected.** Every adapter response read — the + unified chat/embeddings JSON, and the three media endpoints — is capped + at this limit. `audio/speech`'s streaming copy (`openaiAdapter.audioSpeech`, + `provider_openai.go`) already committed the upstream's 200 status before + it starts copying bytes, so a response over the cap simply ends early: no + error, no truncation signal of any kind, a shorter audio file + indistinguishable from a genuinely short one. `images/generations` and + `audio/transcriptions` (`forwardMediaBody`) read their whole response + before writing a status, so they hit the identical cap with the same + silent-truncation-into-200 outcome. 32MiB is generous for any of these + payloads in practice, but nothing enforces or signals the cutoff. - **The response cache shares entries across every user and group that can reach a model.** `cache`'s key (see [Caching](#caching)) carries no user identity by design, so two different callers sending the same diff --git a/admin.go b/admin.go index bb95039..a62504a 100644 --- a/admin.go +++ b/admin.go @@ -5,6 +5,7 @@ import ( "io" "net/http" "net/url" + "strings" "time" ) @@ -22,8 +23,16 @@ const ( // folded review item, 2026-08-20 review): no external assets of any // kind, only this page's own inline script/style, and fetch calls // restricted to same-origin — the dashboard works air-gapped and cannot -// be coerced into loading anything off-host. -const adminCSP = "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'" +// be coerced into loading anything off-host. frame-ancestors/base-uri/ +// form-action are all 'none' (v0.2 final review wave, 2026-08-20): the +// page carries a password-type key-entry input, so it must never be +// embeddable in another site's frame (clickjacking), never have its +// href hijacked to retarget a relative script/fetch URL, and never +// have its auth-form submitted anywhere but nowhere at all — the form's +// own submit handler already intercepts and cancels the real submit +// (adminPageHTML's "submit" listener calls preventDefault()), so +// form-action has nothing legitimate to allow. +const adminCSP = "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'; frame-ancestors 'none'; base-uri 'none'; form-action 'none'" // adminEnabled reports whether cfg's Admin block is present and enabled — // the single gate ServeHTTP checks before matching any /admin* route at @@ -139,6 +148,29 @@ func sanitizeBaseURL(raw string) string { return u.String() } +// sanitizeProviderErr applies sanitizeBaseURL's userinfo/query-stripping +// treatment to lastErr before it is ever echoed by GET /admin/api/overview +// (v0.2 final review wave, 2026-08-20): a provider's discovery/refresh +// error commonly embeds the request URL it failed against (Go's +// net/http wraps a *url.Error carrying it verbatim), so a baseUrl +// misconfigured with embedded credentials would otherwise leak straight +// back out through this field, even though adminProviderView.BaseURL +// itself is already sanitized. +// +// This is best-effort, not a general URL scrubber: it looks only for +// rawBaseURL appearing verbatim inside lastErr and replaces every such +// occurrence with sanitizeBaseURL's cleaned form. An error naming some +// OTHER URL — a redirect target, a differently-formatted variant of the +// same host — is not caught; recognizing every URL shape a wrapped error +// might embed, without risking mangling ordinary error text, is not +// attempted here. +func sanitizeProviderErr(lastErr, rawBaseURL string) string { + if lastErr == "" || rawBaseURL == "" || !strings.Contains(lastErr, rawBaseURL) { + return lastErr + } + return strings.ReplaceAll(lastErr, rawBaseURL, sanitizeBaseURL(rawBaseURL)) +} + // adminProviderView is one provider's read-only view in GET // /admin/api/overview (spec §4, v0.2). baseUrl is not secret — the // spec's NEVER-exposed list is API keys (not even digests), provider @@ -179,6 +211,21 @@ type adminCacheView struct { Enabled bool `json:"enabled"` } +// adminRetryView is the retry config summary in GET /admin/api/overview +// (v0.2 final review wave, 2026-08-20): the EFFECTIVE enabled/attempts/ +// backoff a `retry: {enabled: true}` block resolves to, after +// newRetryPolicy's own zero-value defaulting (retry.go) — not the raw, +// possibly-empty RetryConfig fields, so an operator sees what actually +// governs upstream calls, not just what they left unset. Attempts and +// Backoff are both omitted (zero value / empty string) when retry is +// disabled, mirroring adminCacheView's own omitempty convention for a +// disabled feature's now-meaningless detail fields. +type adminRetryView struct { + Backoff string `json:"backoff,omitempty"` + Enabled bool `json:"enabled"` + Attempts int `json:"attempts,omitempty"` +} + // adminGroupView is one group's summary in GET /admin/api/overview. type adminGroupView struct { Limits *LimitsConfig `json:"limits,omitempty"` @@ -205,6 +252,7 @@ type adminOverviewResponse struct { Aliases []adminAliasView `json:"aliases"` Redis adminRedisView `json:"redis"` Cache adminCacheView `json:"cache"` + Retry adminRetryView `json:"retry"` } // buildAdminOverview assembles adminOverviewResponse from the registry, @@ -220,7 +268,7 @@ func (g *Gateway) buildAdminOverview() adminOverviewResponse { BaseURL: sanitizeBaseURL(s.baseURL), ModelCount: s.modelCount, LastRefresh: s.lastRefresh, - LastErr: s.lastErr, + LastErr: sanitizeProviderErr(s.lastErr, s.baseURL), } } @@ -232,6 +280,19 @@ func (g *Gateway) buildAdminOverview() adminOverviewResponse { ttl = g.cache.ttl.String() } + // newRetryPolicy is pure (no I/O) and already ran once, successfully, + // when this Gateway was constructed (providers.go's buildAdapters) — + // g.cfg.Retry cannot have changed since a running Gateway never + // mutates its own config, so this second call is guaranteed to return + // the identical result with a nil error; the error return is + // intentionally discarded rather than propagated. + rp, _ := newRetryPolicy(g.cfg.Retry) + retryView := adminRetryView{Enabled: rp.enabled} + if rp.enabled { + retryView.Attempts = rp.attempts + retryView.Backoff = rp.backoff.String() + } + _, groupSummaries := g.auth.snapshot() groups := make([]adminGroupView, len(groupSummaries)) for i, gs := range groupSummaries { @@ -248,6 +309,7 @@ func (g *Gateway) buildAdminOverview() adminOverviewResponse { Providers: providers, Redis: adminRedisView{Configured: configured, LastErr: redisLastErr, LastErrAt: redisLastErrAt}, Cache: adminCacheView{Enabled: cacheEnabled, TTL: ttl}, + Retry: retryView, Groups: groups, Aliases: aliases, Version: pluginVersion, @@ -309,25 +371,28 @@ func usageEntryView(su scopeUsage, limits *LimitsConfig) adminUsageEntryView { // buildAdminUsage assembles adminUsageResponse: authStore.snapshot lists // every currently active user and every configured group (names, group // membership, and limits only — never a key or its digest), and -// limiter.currentUsage reads each one's six current-window counters — -// one batched round trip per scope (limits.go's storeGetMulti) rather -// than six separate ones. Unlike buildLimitScopes (routes_unified.go), -// this never omits an entity for having nil limits — the dashboard shows -// usage for every user and group, limited or not. +// limiter.currentUsage reads every one's six current-window counters in +// ONE storeGetMulti round trip total (v0.2 final review wave, 2026-08-20; +// see limiter.currentUsage's own doc comment for the tradeoff this +// supersedes) — users and groups are concatenated into a single scopes +// slice before that one call, then the flat result is sliced back into +// the two response sections at the same split point, order preserved. +// Unlike buildLimitScopes (routes_unified.go), this never omits an +// entity for having nil limits — the dashboard shows usage for every +// user and group, limited or not. func (g *Gateway) buildAdminUsage() adminUsageResponse { userSummaries, groupSummaries := g.auth.snapshot() - userScopes := make([]limitScope, len(userSummaries)) - for i, us := range userSummaries { - userScopes[i] = limitScope{kind: "user", id: us.name, limits: us.limits} + scopes := make([]limitScope, 0, len(userSummaries)+len(groupSummaries)) + for _, us := range userSummaries { + scopes = append(scopes, limitScope{kind: "user", id: us.name, limits: us.limits}) } - groupScopes := make([]limitScope, len(groupSummaries)) - for i, gs := range groupSummaries { - groupScopes[i] = limitScope{kind: "group", id: gs.name, limits: gs.limits} + for _, gs := range groupSummaries { + scopes = append(scopes, limitScope{kind: "group", id: gs.name, limits: gs.limits}) } - userUsage := g.limiter.currentUsage(userScopes) - groupUsage := g.limiter.currentUsage(groupScopes) + allUsage := g.limiter.currentUsage(scopes) + userUsage, groupUsage := allUsage[:len(userSummaries)], allUsage[len(userSummaries):] users := make([]adminUsageEntryView, len(userUsage)) for i, su := range userUsage { @@ -571,6 +636,8 @@ const adminPageHTML = ` infra.appendChild(el("div", redisText, "infra-line")); var cacheText = "Cache: " + (data.cache.enabled ? "enabled (ttl " + data.cache.ttl + ")" : "disabled"); infra.appendChild(el("div", cacheText, "infra-line")); + var retryText = "Retry: " + (data.retry.enabled ? "enabled (attempts " + data.retry.attempts + ", backoff " + data.retry.backoff + ")" : "disabled"); + infra.appendChild(el("div", retryText, "infra-line")); infra.appendChild(el("div", "Version: " + data.version, "infra-line muted")); groupMeta = {}; diff --git a/admin_test.go b/admin_test.go index 43e14b3..72f4e9f 100644 --- a/admin_test.go +++ b/admin_test.go @@ -263,6 +263,12 @@ func TestAdminOverview_SortedShapeAndVersion(t *testing.T) { if got.Redis.Configured { t.Error("redis.configured must be false: cfg.Redis was never configured") } + if got.Retry.Enabled { + t.Error("retry.enabled must be false: cfg.Retry was never configured") + } + if got.Retry.Attempts != 0 || got.Retry.Backoff != "" { + t.Errorf("retry.attempts/backoff must be omitted when disabled, got %+v", got.Retry) + } wantAliases := []adminAliasView{ {Alias: "aaliased/y", Target: "alpha/a-model-1"}, @@ -278,6 +284,38 @@ func TestAdminOverview_SortedShapeAndVersion(t *testing.T) { } } +// TestAdminOverview_RetryEffectiveValues proves GET /admin/api/overview's +// retry block reports the EFFECTIVE, defaulted values newRetryPolicy +// (retry.go) resolves an enabled retry block to, not the raw config — +// Attempts and Backoff are both left at their zero value here specifically +// to exercise that defaulting (v0.2 final review wave, 2026-08-20). +func TestAdminOverview_RetryEffectiveValues(t *testing.T) { + t.Parallel() + cfg := newAdminTestConfig() + cfg.Retry = RetryConfig{Enabled: true} + h, _ := newAdminGatewayHandle(t, cfg) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, adminRequest(http.MethodGet, adminOverviewPath, "sk-admin1")) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + var got adminOverviewResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if !got.Retry.Enabled { + t.Fatal("retry.enabled must be true") + } + if got.Retry.Attempts != defaultRetryAttempts { + t.Errorf("retry.attempts = %d, want the default %d (raw config left Attempts at 0)", got.Retry.Attempts, defaultRetryAttempts) + } + if got.Retry.Backoff != defaultRetryBackoff { + t.Errorf("retry.backoff = %q, want the default %q (raw config left Backoff empty)", got.Retry.Backoff, defaultRetryBackoff) + } +} + func TestAdminOverview_ProviderLastErrAfterFailedRefresh(t *testing.T) { t.Parallel() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -478,7 +516,7 @@ func TestAdminPage_CSPHeaderAndFetchURLs(t *testing.T) { if ct := rec.Header().Get("Content-Type"); !strings.Contains(ct, "text/html") { t.Errorf("content-type = %q, want text/html", ct) } - wantCSP := "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'" + wantCSP := "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'; frame-ancestors 'none'; base-uri 'none'; form-action 'none'" if got := rec.Header().Get("Content-Security-Policy"); got != wantCSP { t.Errorf("CSP = %q, want %q", got, wantCSP) } @@ -506,7 +544,7 @@ func TestAdmin_JSONSecurityHeaders(t *testing.T) { cfg := newAdminTestConfig() h, _ := newAdminGatewayHandle(t, cfg) - wantCSP := "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'" + wantCSP := "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'; frame-ancestors 'none'; base-uri 'none'; form-action 'none'" for _, p := range []string{adminOverviewPath, adminUsagePath} { t.Run(p, func(t *testing.T) { rec := httptest.NewRecorder() @@ -660,8 +698,9 @@ func TestAdminOverview_BaseURLStripsCredentials(t *testing.T) { // countingMultiStore is a counterStore stub whose getMulti serves a fixed // value per key and counts how many times it was called — it proves -// limiter.currentUsage batches a scope's six counter reads into one -// getMulti call rather than issuing them as six separate get calls. +// limiter.currentUsage flattens every scope's six counter reads into ONE +// getMulti call total (v0.2 final review wave, 2026-08-20), rather than +// six separate get calls or even one getMulti call per scope. // incrBy/get are never exercised by currentUsage and just return zero // values. type countingMultiStore struct { @@ -682,7 +721,7 @@ func (s *countingMultiStore) getMulti(keys []string) ([]int64, error) { return out, nil } -func TestLimiterCurrentUsage_BatchesOneGetMultiCallPerScope(t *testing.T) { +func TestLimiterCurrentUsage_BatchesOneGetMultiCallForAllScopes(t *testing.T) { t.Parallel() fixedNow := time.Date(2026, 8, 20, 10, 0, 0, 0, time.UTC) store := &countingMultiStore{values: map[string]int64{ @@ -703,8 +742,8 @@ func TestLimiterCurrentUsage_BatchesOneGetMultiCallPerScope(t *testing.T) { } got := l.currentUsage(scopes) - if store.getMultiCalls != len(scopes) { - t.Errorf("getMultiCalls = %d, want %d (one pipelined getMulti per scope, not six separate get calls)", store.getMultiCalls, len(scopes)) + if store.getMultiCalls != 1 { + t.Errorf("getMultiCalls = %d, want 1 (all scopes' keys flattened into one pipelined getMulti call)", store.getMultiCalls) } if len(got) != 2 { t.Fatalf("len(got) = %d, want 2", len(got)) diff --git a/integration/integration_test.go b/integration/integration_test.go index e32eb6d..cd1ce89 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -21,6 +21,7 @@ import ( "bytes" "encoding/json" "io" + "mime/multipart" "net/http" "os" "os/exec" @@ -89,6 +90,61 @@ func doJSON(t *testing.T, method, url, apiKey string, body any) (*http.Response, return resp, out } +// buildMultipartAudio builds a multipart/form-data body carrying a dummy +// "file" part and a "model" part set to model, mirroring what an +// OpenAI-SDK client sends to POST /v1/audio/transcriptions. It returns the +// encoded body and its matching Content-Type (including the boundary +// parameter multipart.Writer generated). +func buildMultipartAudio(t *testing.T, model string) ([]byte, string) { + t.Helper() + var buf bytes.Buffer + mw := multipart.NewWriter(&buf) + fw, err := mw.CreateFormFile("file", "audio.wav") + if err != nil { + t.Fatalf("create form file part: %v", err) + } + if _, err := fw.Write([]byte("mock-audio-bytes")); err != nil { + t.Fatalf("write file part: %v", err) + } + if err := mw.WriteField("model", model); err != nil { + t.Fatalf("write model field: %v", err) + } + if err := mw.Close(); err != nil { + t.Fatalf("close multipart writer: %v", err) + } + return buf.Bytes(), mw.FormDataContentType() +} + +// doMultipart issues one multipart/form-data POST, mirroring doJSON's +// contract: the returned *http.Response has already had its body read and +// closed, and a non-JSON or empty body decodes to a nil map. +func doMultipart(t *testing.T, url, apiKey, contentType string, body []byte) (*http.Response, map[string]any) { + t.Helper() + + req, err := http.NewRequest(http.MethodPost, url, bytes.NewReader(body)) + if err != nil { + t.Fatalf("new request POST %s: %v", url, err) + } + req.Header.Set("Authorization", "Bearer "+apiKey) + req.Header.Set("Content-Type", contentType) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("do request POST %s: %v", url, err) + } + defer func() { _ = resp.Body.Close() }() + + raw, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read response body from POST %s: %v", url, err) + } + var out map[string]any + if len(raw) > 0 { + _ = json.Unmarshal(raw, &out) + } + return resp, out +} + // firstChoiceContent extracts choices[0].message.content from an // OpenAI-shaped chat completion response body, failing the test if the // shape doesn't match. @@ -827,6 +883,39 @@ func TestAudioSpeech(t *testing.T) { } } +// TestAudioTranscriptions covers the review wave's highest-value Yaegi gap: +// POST /v1/audio/transcriptions' multipart handling under real +// Traefik+Yaegi. A provider-prefixed model ("openai/gpt-mock") drives +// rewriteMultipartModel (routes_media.go), which rebuilds the multipart +// body with the "model" part rewritten to the resolved bare upstream id +// before it reaches the mock — asserted via mock/main.go's +// handleOpenAIAudioTranscriptions echoing "model_received" back. A second, +// already-bare request ("gpt-mock") exercises the byte-identical replay +// path instead: no rebuild, same bare id reported back verbatim. +func TestAudioTranscriptions(t *testing.T) { + t.Run("provider-prefixed model is rewritten before upload", func(t *testing.T) { + body, contentType := buildMultipartAudio(t, "openai/gpt-mock") + resp, decoded := doMultipart(t, traefik1URL+"/v1/audio/transcriptions", aliceKey, contentType, body) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, body=%#v", resp.StatusCode, decoded) + } + if got, _ := decoded["model_received"].(string); got != "gpt-mock" { + t.Errorf("model_received = %q, want %q (rewritten from the provider-prefixed id)", got, "gpt-mock") + } + }) + + t.Run("bare model replays the original body unchanged", func(t *testing.T) { + body, contentType := buildMultipartAudio(t, "gpt-mock") + resp, decoded := doMultipart(t, traefik1URL+"/v1/audio/transcriptions", aliceKey, contentType, body) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, body=%#v", resp.StatusCode, decoded) + } + if got, _ := decoded["model_received"].(string); got != "gpt-mock" { + t.Errorf("model_received = %q, want %q (byte-identical replay, no rewrite needed)", got, "gpt-mock") + } + }) +} + // TestAdminDashboard covers v0.2 integration coverage (spec §4): the // unauthenticated HTML shell carries no data, the two JSON routes gate // on admin vs. non-admin vs. unauthenticated, and GET /admin/api/usage diff --git a/integration/mock/main.go b/integration/mock/main.go index fc617b3..1c1a59b 100644 --- a/integration/mock/main.go +++ b/integration/mock/main.go @@ -84,6 +84,7 @@ func newServerMux(mode string) (*http.ServeMux, error) { mux.HandleFunc("/flaky/reset", handleFlakyReset) mux.HandleFunc("/v1/images/generations", handleOpenAIImages) mux.HandleFunc("/v1/audio/speech", handleOpenAIAudioSpeech) + mux.HandleFunc("/v1/audio/transcriptions", handleOpenAIAudioTranscriptions) case "anthropic": mux.HandleFunc("/v1/models", handleAnthropicModels) mux.HandleFunc("/v1/messages", handleAnthropicMessages) @@ -309,6 +310,27 @@ func handleOpenAIAudioSpeech(w http.ResponseWriter, r *http.Request) { _, _ = w.Write(mockAudioBytes) } +// handleOpenAIAudioTranscriptions answers POST /v1/audio/transcriptions by +// parsing the multipart body Traefik forwarded and echoing back the "model" +// form field it finds — this is the review wave's highest-value Yaegi +// coverage gap closed: the audio-transcriptions integration test sends a +// provider-prefixed model ("openai/gpt-mock") and asserts this handler +// receives the bare, rewritten form ("gpt-mock"), proving +// rewriteMultipartModel (routes_media.go) actually rebuilds the multipart +// body under real Traefik+Yaegi, not just in the compiled unit tests. A +// second, already-bare request exercises the byte-identical replay path +// instead, so this same field still reads back "gpt-mock" with no rebuild. +func handleOpenAIAudioTranscriptions(w http.ResponseWriter, r *http.Request) { + if err := r.ParseMultipartForm(1 << 20); err != nil { //nolint:gosec // G120: maxMemory is set (1MiB) and the whole handler is behind readCapped's 10MiB gateway-side cap (routes_media.go) before this mock ever sees the body; test-only fixture server, not production code + http.Error(w, `{"error":{"message":"invalid multipart body","type":"invalid_request_error"}}`, http.StatusBadRequest) + return + } + writeJSON(w, map[string]any{ + "text": "mock transcription", + "model_received": r.FormValue("model"), + }) +} + // --- anthropic mode ---------------------------------------------------------- func handleAnthropicModels(w http.ResponseWriter, _ *http.Request) { diff --git a/limits.go b/limits.go index 2571435..b68f4e1 100644 --- a/limits.go +++ b/limits.go @@ -723,42 +723,60 @@ func usageWindowKeys(sc limitScope, now time.Time) []string { // does, and it is read-only: unlike checkAndCount, it never increments // anything. // -// One storeGetMulti call per scope (controller-approved amendment, -// 2026-08-20 review): the original per-metric l.getCounter loop issued -// six separate round trips per scope against the single shared Redis -// connection — GET /admin/api/usage serialized 6×(users+groups) of them. -// Batching all scopes' keys into one single storeGetMulti call would cut -// that further, to one round trip total, but was not taken here: it -// would need buildAdminUsage to flatten and later re-split two -// differently-sized scope lists (users, groups) around one shared call, -// for a further win only realized when both lists are non-trivially -// large. One call per scope already turns 6N round trips into N, keeps -// currentUsage's per-scope iteration shape identical to -// checkAndCount/evaluateScope's, and keeps a single scope's store -// failure from forcing every other scope in the same request to look -// storeDown too. Order is preserved: currentUsage(scopes)[i] corresponds -// to scopes[i]. +// ONE storeGetMulti call for the whole scopes slice (v0.2 final review +// wave, 2026-08-20; supersedes the "one call per scope" amendment this +// comment previously described, 2026-08-20 review): every scope's six +// keys are flattened into a single round trip against the shared store, +// rather than N separate ones. The reviewer-confirmed reason for this +// second amendment: GET /admin/api/usage's per-scope loop was contending +// with request admission on the same store connection as live traffic, +// and N round trips (even pipelined 6-at-a-time) scales with the number +// of configured users and groups in a way one round trip does not. +// buildAdminUsage (admin.go) drives this by concatenating its users and +// groups into one scopes slice before calling in, then slicing the flat +// result back into its two response sections at the same split point. +// +// Tradeoff given up by this second amendment: a transient store error +// now marks every scope in the call storeDown together, where the +// previous per-scope loop could let one scope's failure land mid-batch +// while a later scope's own independent call still succeeded. That +// isolation was never a documented guarantee of this method, only an +// incidental property of how it was written — cutting N round trips to 1 +// is the reviewer-confirmed right side of this tradeoff. Order is +// preserved: currentUsage(scopes)[i] corresponds to scopes[i]. func (l *limiter) currentUsage(scopes []limitScope) []scopeUsage { - now := l.now() out := make([]scopeUsage, len(scopes)) - for i, sc := range scopes { - keys := usageWindowKeys(sc, now) - vals, ok := l.storeGetMulti(keys) - // len(vals) != len(keys) is reachable only from a non-conforming - // counterStore implementation (a real respClient.getMulti and - // memoryStore.getMulti both always return one value per key) — - // guarded defensively so a future or test-only store's short - // slice reports storeDown instead of panicking on an - // out-of-range index below (review sweep, 2026-08-20). - if !ok || len(vals) != len(keys) { + if len(scopes) == 0 { + return out + } + + now := l.now() + allKeys := make([]string, 0, len(scopes)*6) + for _, sc := range scopes { + allKeys = append(allKeys, usageWindowKeys(sc, now)...) + } + + vals, ok := l.storeGetMulti(allKeys) + // len(vals) != len(allKeys) is reachable only from a non-conforming + // counterStore implementation (a real respClient.getMulti and + // memoryStore.getMulti both always return one value per key) — + // guarded defensively so a future or test-only store's short slice + // reports every scope storeDown instead of panicking on an + // out-of-range index below (review sweep, 2026-08-20). + if !ok || len(vals) != len(allKeys) { + for i, sc := range scopes { out[i] = scopeUsage{kind: sc.kind, id: sc.id, storeDown: true} - continue } + return out + } + + for i, sc := range scopes { + v := vals[i*6 : i*6+6] out[i] = scopeUsage{ kind: sc.kind, id: sc.id, - requestsPerMinute: vals[0], requestsPerDay: vals[1], - tokensPerDay: vals[2], tokensPerMonth: vals[3], - costPerDayMicros: vals[4], costPerMonthMicros: vals[5], + requestsPerMinute: v[0], requestsPerDay: v[1], + tokensPerDay: v[2], tokensPerMonth: v[3], + costPerDayMicros: v[4], costPerMonthMicros: v[5], } } return out