From f0730a70d7c20e40185a34ee141fbddf0e4a8805 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 20 Aug 2026 21:06:00 +0100 Subject: [PATCH] fix(aliases): deterministic integration gate, warn-first fast path, deduped listing Review fix, task 6 (1 Important + 3 folded): 1. TestModelAliases now polls GET /v1/models (waitForModelAlias) until "aliased/mock" appears, bounded 30s, before asserting - removes the stale-dynamic-config propagation window a plain healthy-200 check cannot rule out. Confirmed config-propagation, not alias-logic, per reviewer's config+error-text analysis. 2. registry.go resolve(): check the warned-set (aliasShadowWarned) before paying bareWinner's O(providerNames) scan on every aliased request - only the first request for a colliding alias pays that cost now. 3. listFor: skip an alias entry whose id already exists in owners (a real model already owns it) - previously produced duplicate ids when a discovered model collided with a configured alias. resolve's own alias-wins precedence is unchanged; only the listing is deduped. New test: TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision. 4. newAliasTestRegistry doc comment: "grp's given aliases" -> "the given aliases" (no grp parameter exists). --- integration/integration_test.go | 36 +++++++++++++++++++ registry.go | 61 +++++++++++++++++++++++++-------- registry_test.go | 59 ++++++++++++++++++++++++++++++- 3 files changed, 141 insertions(+), 15 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index 9b08971..e32eb6d 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -139,6 +139,37 @@ func waitForHealthy(t *testing.T, url, apiKey string, timeout time.Duration) { t.Fatalf("%s did not become healthy within %s (last status=%d, last error=%v)", url, timeout, lastStatus, lastErr) } +// waitForModelAlias polls GET /v1/models on baseURL with apiKey until an +// entry with id wantID appears, or timeout elapses. Task 6 review fix: a +// plain 200 from /v1/models (waitForHealthy's own check) only proves +// Traefik's file provider has loaded SOME generation of dynamic.yml — not +// necessarily the one carrying modelAliases. A request landing in that +// narrow propagation window would see a config generation from before the +// alias applied and get a spurious "unknown model" 404, which is exactly +// what one cold-start run of this suite hit. Polling the actual listing +// removes that window deterministically instead of guessing at a fixed +// sleep. +func waitForModelAlias(t *testing.T, baseURL, apiKey, wantID string, timeout time.Duration) { + t.Helper() + deadline := time.Now().Add(timeout) + var lastBody map[string]any + for time.Now().Before(deadline) { + resp, body := doJSON(t, http.MethodGet, baseURL+"/v1/models", apiKey, nil) + if resp.StatusCode == http.StatusOK { + lastBody = body + if data, ok := body["data"].([]any); ok { + for _, entry := range data { + if m, ok := entry.(map[string]any); ok && m["id"] == wantID { + return + } + } + } + } + time.Sleep(500 * time.Millisecond) + } + t.Fatalf("GET %s/v1/models never listed alias %q within %s, last body=%#v", baseURL, wantID, timeout, lastBody) +} + // checkNoPluginErrors greps docker compose's captured logs for service, // failing if no plugin-loading message appears at all, or if any // plugin-related line is logged at error level. @@ -248,6 +279,11 @@ func TestUnifiedChatAllProviders(t *testing.T) { // registry_test.go's Gateway-level equivalents for the same assertion // under go test). func TestModelAliases(t *testing.T) { + // Review fix: wait for the alias to actually appear in /v1/models + // before asserting anything below — see waitForModelAlias's doc + // comment for why a plain healthy-200 is not enough on its own. + waitForModelAlias(t, traefik1URL, aliceKey, "aliased/mock", 30*time.Second) + // openai-type: the upstream response is forwarded verbatim // (provider_openai.go), so its "model" field carries whatever the // mock echoes back — the resolved upstream id "gpt-mock", not the diff --git a/registry.go b/registry.go index e5f62f4..a3cfea4 100644 --- a/registry.go +++ b/registry.go @@ -466,8 +466,15 @@ func (m *modelRegistry) bareWinner(id string) (string, bool) { // errModelUnknown for a model no configured provider knows at all. func (m *modelRegistry) resolve(id string, grp *group) (providerAdapter, string, string, error) { if target, isAlias := m.aliases[id]; isAlias { - if _, discoveredCollision := m.bareWinner(id); discoveredCollision { - m.warnAliasShadowsDiscoveredOnce(id) + // Warned-set checked FIRST (review fix): bareWinner is an + // O(providerNames) scan, and every request for an already-warned + // alias would otherwise pay it just to decide not to log again. + // Once warned, this collapses to one cheap mutex-guarded map + // lookup per request instead. + if !m.aliasShadowWarned(id) { + if _, discoveredCollision := m.bareWinner(id); discoveredCollision { + m.warnAliasShadowsDiscoveredOnce(id) + } } return m.resolveAliasTarget(id, target, grp) } @@ -529,6 +536,27 @@ func (m *modelRegistry) resolveAliasTarget(alias, target string, grp *group) (pr return m.resolveAgainst(providerName, target, target, alias, grp, notFound) } +// aliasShadowWarnKey builds the m.warned key for alias's discovered- +// collision warning — shared by aliasShadowWarned and +// warnAliasShadowsDiscoveredOnce so the two can never drift apart into +// mismatched keys. Prefixed with "alias-shadow:" so it can never collide +// with warnCollisionOnce's own plain-id keys in the same m.warned map. +func aliasShadowWarnKey(alias string) string { + return "alias-shadow:" + alias +} + +// aliasShadowWarned reports whether warnAliasShadowsDiscoveredOnce has +// already logged for alias, with no other side effect — resolve's fast +// path (its own doc comment) checks this BEFORE paying bareWinner's +// O(providerNames) scan, so a registry with many providers does not pay +// that cost on every request for an alias once its one-time warning has +// already fired. +func (m *modelRegistry) aliasShadowWarned(alias string) bool { + m.warnedMu.Lock() + defer m.warnedMu.Unlock() + return m.warned[aliasShadowWarnKey(alias)] +} + // warnAliasShadowsDiscoveredOnce logs, at most once per colliding alias id // for this registry's lifetime, that alias also names a model some // provider's discovery fetch found after construction — resolve's @@ -536,13 +564,11 @@ func (m *modelRegistry) resolveAliasTarget(alias, target string, grp *group) (pr // wins, since only an EXPLICIT collision is rejected at construction // (validateModelAliases); discovery is dynamic and runs after that check, // so this collision can only be caught, and only warned about, here at -// request time. Keyed with an "alias-shadow:" prefix in the same m.warned -// map warnCollisionOnce uses, so the two unrelated warning kinds can never -// share a key and suppress each other. +// request time. func (m *modelRegistry) warnAliasShadowsDiscoveredOnce(alias string) { m.warnedMu.Lock() defer m.warnedMu.Unlock() - key := "alias-shadow:" + alias + key := aliasShadowWarnKey(alias) if m.warned[key] { return } @@ -668,15 +694,22 @@ func (m *modelRegistry) listFor(grp *group) []map[string]any { // then deny. owned_by is recovered from the successful call's own // canonical return value ("provider/upstreamModel") by splitting on // the first "/" — exact, because a registry provider name can never - // itself contain one (configNamePattern, providers.go). An alias id - // that also happens to collide with a since-discovered model's own - // bare id (impossible for an EXPLICIT model — validateModelAliases - // already rejects that at construction) is not deduped against that - // model's own entry above: the same documented, alias-wins - // precedence edge resolve's warnAliasShadowsDiscoveredOnce logs - // about, accepted here too as rare enough not to warrant extra - // bookkeeping. + // itself contain one (configNamePattern, providers.go). + // + // An alias id that also happens to collide with a since-discovered + // model's own bare id (impossible for an EXPLICIT model — + // validateModelAliases already rejects that at construction) is + // SKIPPED here (review fix), not appended a second time: owners + // already holds a real-model entry for that id from the loop above, + // and adding another with the same id would list it twice — + // duplicate, order-nondeterministic-looking entries in an + // OpenAI-shaped model list. resolve's own precedence (its doc + // comment) still has the alias win at REQUEST time regardless of + // which entry the listing shows; only the listing itself is deduped. for alias, target := range m.aliases { + if _, collides := owners[alias]; collides { + continue + } _, _, canonical, err := m.resolveAliasTarget(alias, target, grp) if err != nil { continue diff --git a/registry_test.go b/registry_test.go index 2f7117f..cb84ed0 100644 --- a/registry_test.go +++ b/registry_test.go @@ -899,7 +899,7 @@ func TestNewGateway_WarmFill_PopulatesDiscoveredModels_BeforeAnyRequest(t *testi // --- model aliases (spec §5, v0.2) --- // newAliasTestRegistry builds a registry with one "openai" provider -// (explicit model "gpt-test") and grp's given aliases, failing the test on +// (explicit model "gpt-test") and the given aliases, failing the test on // any construction error. log defaults to a no-op when nil. func newAliasTestRegistry(t *testing.T, aliases map[string]string, log func(string, ...any)) *modelRegistry { t.Helper() @@ -1243,3 +1243,60 @@ func TestModelRegistry_ListFor_Alias_NotAuthorized_Omitted(t *testing.T) { } } } + +// TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision is the +// review fix for listFor's alias-listing loop: when a provider's +// discovery fetch finds a model whose bare id is identical to a +// configured alias id (impossible for an EXPLICIT model — +// validateModelAliases already rejects that at construction, but +// discovery is dynamic and runs after that check), the listing must show +// exactly ONE entry for that id — not one from the real discovered model +// and a second, duplicate one from the alias loop. resolve's own +// precedence (registry.go's resolve doc comment) must still have the +// alias win at request time regardless of which entry the listing shows. +func TestModelRegistry_ListFor_Alias_DedupedAgainstDiscoveredCollision(t *testing.T) { + t.Parallel() + adapters := map[string]providerAdapter{ + "openai": newFakeAdapter("openai"), + "acme": newFakeAdapter("acme"), + } + cfg := &Config{ + Providers: map[string]*ProviderConfig{ + "openai": {Models: []string{"gpt-test"}}, + "acme": {Discovery: true}, + }, + ModelAliases: map[string]string{"shared-id": "openai/gpt-test"}, + } + reg, err := newModelRegistry(adapters, cfg, func(string, ...any) {}) + if err != nil { + t.Fatalf("newModelRegistry: %v", err) + } + // acme's discovery finds a model literally named "shared-id" — the + // same string as the configured alias, discovered only after + // construction, so validateModelAliases never saw it. + reg.states["acme"].finishRefresh(reg.now(), []string{"shared-id"}, nil) + + got := reg.listFor(allowAllGroup()) + count := 0 + for _, entry := range got { + if entry["id"] == "shared-id" { + count++ + } + } + if count != 1 { + t.Errorf("listFor has %d entries for id %q, want exactly 1 (deduped)", count, "shared-id") + } + + // resolve must still prefer the alias, per its documented precedence, + // regardless of which single entry the listing showed above. + adapter, _, canonical, err := reg.resolve("shared-id", allowAllGroup()) + if err != nil { + t.Fatalf("resolve: %v", err) + } + if adapter != adapters["openai"] { + t.Errorf("adapter = %v, want openai (the alias, not acme's discovered model, must still win resolution)", adapter) + } + if canonical != "openai/gpt-test" { + t.Errorf("canonical = %q, want %q", canonical, "openai/gpt-test") + } +}