From ed879f0f683168b9707615f181195998c74ad360 Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Mon, 24 Aug 2026 12:33:37 +0200 Subject: [PATCH 1/2] perf(models): fetch the model list conditionally with ETag Each hourly refresh re-downloaded and reparsed the full external model list (~1.4MB raw, ~10-15MB of allocation churn) even though upstream changes about once a day, ratcheting the Go heap high-water mark and process RSS by ~20MB. Send If-None-Match on refreshes and skip the download, reparse, re-enrichment, and cache save on 304 Not Modified. The validator is persisted in the model cache so warm restarts skip the initial download too. Servers without ETag support keep answering 200 and degrade to the previous unconditional behavior. --- internal/cache/modelcache/modelcache.go | 3 + internal/modeldata/fetcher.go | 45 +++++++++-- internal/modeldata/fetcher_test.go | 92 +++++++++++++++++++++++ internal/providers/init.go | 21 +++--- internal/providers/registry.go | 1 + internal/providers/registry_cache.go | 3 + internal/providers/registry_cache_test.go | 38 ++++++++++ internal/providers/registry_init.go | 33 +++++--- internal/providers/registry_metadata.go | 27 ++++++- internal/providers/registry_test.go | 77 +++++++++++++++++++ 10 files changed, 311 insertions(+), 29 deletions(-) diff --git a/internal/cache/modelcache/modelcache.go b/internal/cache/modelcache/modelcache.go index b113fe500..472d6a466 100644 --- a/internal/cache/modelcache/modelcache.go +++ b/internal/cache/modelcache/modelcache.go @@ -19,6 +19,9 @@ type ModelCache struct { // ModelListData holds the raw JSON model registry bytes for cache persistence, // allowing the registry to restore its full model list without re-fetching. ModelListData json.RawMessage `json:"model_list_data,omitempty"` + // ModelListETag is the HTTP validator ModelListData was downloaded with, + // letting the next fetch skip the download when the list is unchanged. + ModelListETag string `json:"model_list_etag,omitempty"` } // CachedProvider holds shared fields for all models from a single provider. diff --git a/internal/modeldata/fetcher.go b/internal/modeldata/fetcher.go index 6ae84445c..ba2fc4add 100644 --- a/internal/modeldata/fetcher.go +++ b/internal/modeldata/fetcher.go @@ -17,49 +17,78 @@ var httpClient = &http.Client{ Timeout: 60 * time.Second, } +// FetchResult carries the outcome of one conditional model list fetch. +type FetchResult struct { + List *ModelList + Raw []byte + // ETag is the validator to send on the next conditional fetch. Empty when + // the server did not return one. + ETag string + // NotModified is true when the server answered 304 for the presented ETag; + // List and Raw are nil and the caller keeps its current data. + NotModified bool +} + // Fetch downloads and parses the model list from the given URL. // Returns the parsed ModelList, the raw JSON bytes (for caching), and any error. // Returns nil, nil, nil if the URL is empty (feature disabled). // The caller controls timeout via the provided context (e.g. context.WithTimeout). func Fetch(ctx context.Context, url string) (*ModelList, []byte, error) { + result, err := FetchIfChanged(ctx, url, "") + return result.List, result.Raw, err +} + +// FetchIfChanged downloads and parses the model list unless the server reports +// it unchanged. When etag is non-empty it is sent as If-None-Match; a 304 +// response returns NotModified=true with the etag carried forward, skipping +// the download and reparse entirely. Servers without ETag support keep +// answering 200, so callers transparently degrade to unconditional fetching. +// Returns a zero FetchResult and nil error if the URL is empty (feature disabled). +func FetchIfChanged(ctx context.Context, url, etag string) (FetchResult, error) { if url == "" { - return nil, nil, nil + return FetchResult{}, nil } client := httpClient req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { - return nil, nil, fmt.Errorf("creating request: %w", err) + return FetchResult{}, fmt.Errorf("creating request: %w", err) } req.Header.Set("Accept", "application/json") + if etag != "" { + req.Header.Set("If-None-Match", etag) + } resp, err := client.Do(req) if err != nil { - return nil, nil, fmt.Errorf("fetching model list: %w", err) + return FetchResult{}, fmt.Errorf("fetching model list: %w", err) } defer resp.Body.Close() + if etag != "" && resp.StatusCode == http.StatusNotModified { + return FetchResult{ETag: etag, NotModified: true}, nil + } if resp.StatusCode != http.StatusOK { - return nil, nil, fmt.Errorf("unexpected status %d from %s", resp.StatusCode, url) + return FetchResult{}, fmt.Errorf("unexpected status %d from %s", resp.StatusCode, url) } const maxBodySize = 10 * 1024 * 1024 // 10 MB limited := io.LimitReader(resp.Body, maxBodySize+1) raw, err := io.ReadAll(limited) if err != nil { - return nil, nil, fmt.Errorf("reading response body: %w", err) + return FetchResult{}, fmt.Errorf("reading response body: %w", err) } if len(raw) > maxBodySize { - return nil, nil, fmt.Errorf("response body too large (exceeds %d bytes)", maxBodySize) + return FetchResult{}, fmt.Errorf("response body too large (exceeds %d bytes)", maxBodySize) } list, err := Parse(raw) if err != nil { - return nil, nil, err + return FetchResult{}, err } - return list, raw, nil + return FetchResult{List: list, Raw: raw, ETag: resp.Header.Get("ETag")}, nil } // Parse deserializes raw JSON bytes into a ModelList. diff --git a/internal/modeldata/fetcher_test.go b/internal/modeldata/fetcher_test.go index cde2a3f9e..bf1807b2b 100644 --- a/internal/modeldata/fetcher_test.go +++ b/internal/modeldata/fetcher_test.go @@ -114,6 +114,98 @@ func TestFetch_OversizedBody(t *testing.T) { } } +func TestFetchIfChanged_CapturesETag(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("If-None-Match"); got != "" { + t.Errorf("unexpected If-None-Match header %q on unconditional fetch", got) + } + w.Header().Set("ETag", `"abc123"`) + _, _ = w.Write([]byte(`{"version": 1, "providers": {}, "models": {}, "provider_models": {}}`)) + })) + defer server.Close() + + result, err := FetchIfChanged(context.Background(), server.URL, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.NotModified { + t.Error("expected NotModified=false for 200 response") + } + if result.List == nil || result.Raw == nil { + t.Fatal("expected list and raw bytes") + } + if result.ETag != `"abc123"` { + t.Errorf("ETag = %q, want %q", result.ETag, `"abc123"`) + } +} + +func TestFetchIfChanged_NotModified(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("If-None-Match"); got != `"abc123"` { + t.Errorf("If-None-Match = %q, want %q", got, `"abc123"`) + } + w.WriteHeader(http.StatusNotModified) + })) + defer server.Close() + + result, err := FetchIfChanged(context.Background(), server.URL, `"abc123"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !result.NotModified { + t.Fatal("expected NotModified=true for 304 response") + } + if result.List != nil || result.Raw != nil { + t.Error("expected nil list and raw on 304") + } + if result.ETag != `"abc123"` { + t.Errorf("ETag = %q, want the presented validator carried forward", result.ETag) + } +} + +func TestFetchIfChanged_ChangedContent(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"v2"`) + _, _ = w.Write([]byte(`{"version": 2, "providers": {}, "models": {}, "provider_models": {}}`)) + })) + defer server.Close() + + result, err := FetchIfChanged(context.Background(), server.URL, `"v1"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.NotModified { + t.Error("expected NotModified=false when content changed") + } + if result.List == nil || result.List.Version != 2 { + t.Fatal("expected updated list") + } + if result.ETag != `"v2"` { + t.Errorf("ETag = %q, want %q", result.ETag, `"v2"`) + } +} + +func TestFetchIfChanged_ServerWithoutETagSupport(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"version": 1, "providers": {}, "models": {}, "provider_models": {}}`)) + })) + defer server.Close() + + result, err := FetchIfChanged(context.Background(), server.URL, `"stale"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.NotModified { + t.Error("expected NotModified=false when server ignores validators") + } + if result.List == nil { + t.Fatal("expected list from 200 response") + } + if result.ETag != "" { + t.Errorf("ETag = %q, want empty when server returns none", result.ETag) + } +} + func TestParse_ValidJSON(t *testing.T) { raw := []byte(`{ "version": 1, diff --git a/internal/providers/init.go b/internal/providers/init.go index a71545649..1ea94f21f 100644 --- a/internal/providers/init.go +++ b/internal/providers/init.go @@ -65,8 +65,8 @@ func (r *InitResult) Close() error { // 2. Cache initialization (local or Redis based on config) // 3. Provider instantiation and registration // 4. Async model loading (from cache first, then network refresh) -// 5. Best-effort background model-list fetch (goroutine with ~45s timeout that -// calls modeldata.Fetch, registry.EnrichModels, and SaveToCache) +// 5. Best-effort background model-list fetch (goroutine with ~45s timeout; +// conditional via ETag, then enrich and SaveToCache when content changed) // 6. Background refresh scheduling (interval from cfg.Cache.RefreshInterval) // 7. Router creation // @@ -133,25 +133,28 @@ func Init(ctx context.Context, result *config.LoadResult, factory *ProviderFacto fetchCtx, cancel := context.WithTimeout(ctx, 45*time.Second) defer cancel() - list, raw, err := modeldata.Fetch(fetchCtx, modelListURL) + result, err := modeldata.FetchIfChanged(fetchCtx, modelListURL, registry.currentModelListETag()) if err != nil { slog.Warn("failed to fetch model list", "url", modelListURL, "error", err) return } - if list == nil { + if result.NotModified { + slog.Info("model list unchanged since last download, using cached copy") + return + } + if result.List == nil { return } - registry.SetModelList(list, raw) - metadataStats := registry.enrichModels() + metadataStats := registry.setModelListAndEnrich(result.List, result.Raw, result.ETag) if err := registry.SaveToCache(fetchCtx); err != nil { slog.Warn("failed to save cache after model list fetch", "error", err) } attrs := []any{ - "models", len(list.Models), - "providers", len(list.Providers), - "provider_models", len(list.ProviderModels), + "models", len(result.List.Models), + "providers", len(result.List.Providers), + "provider_models", len(result.List.ProviderModels), } attrs = append(attrs, metadataStats.slogAttrs()...) slog.Info("model list loaded", attrs...) diff --git a/internal/providers/registry.go b/internal/providers/registry.go index baa6fe6b1..e71dce178 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -62,6 +62,7 @@ type ModelRegistry struct { refreshOnce sync.Once // initializes refreshCh for zero-value safety modelList *modeldata.ModelList // parsed model list (nil = not loaded) modelListRaw json.RawMessage // raw bytes for cache persistence + modelListETag string // validator for conditional refetches; empty = fetch unconditionally // configMetadataOverrides holds operator-supplied metadata keyed by provider // instance name -> raw model ID. Applied after remote-registry enrichment as // a higher-priority layer. nil if no overrides declared. diff --git a/internal/providers/registry_cache.go b/internal/providers/registry_cache.go index c83fadee6..da8dc4840 100644 --- a/internal/providers/registry_cache.go +++ b/internal/providers/registry_cache.go @@ -134,6 +134,7 @@ func (r *ModelRegistry) LoadFromCache(ctx context.Context) (int, error) { if list != nil { r.modelList = list r.modelListRaw = modelCache.ModelListData + r.modelListETag = modelCache.ModelListETag } r.mu.Unlock() @@ -166,6 +167,7 @@ func (r *ModelRegistry) SaveToCache(ctx context.Context) error { providerTypes := make(map[core.Provider]string, len(r.providerTypes)) maps.Copy(providerTypes, r.providerTypes) modelListRaw := r.modelListRaw + modelListETag := r.modelListETag r.mu.RUnlock() if cacheBackend == nil { @@ -176,6 +178,7 @@ func (r *ModelRegistry) SaveToCache(ctx context.Context) error { UpdatedAt: time.Now().UTC(), Providers: make(map[string]modelcache.CachedProvider, len(modelsByProvider)), ModelListData: modelListRaw, + ModelListETag: modelListETag, } var totalModels int diff --git a/internal/providers/registry_cache_test.go b/internal/providers/registry_cache_test.go index dcdef6b40..d58dbd2be 100644 --- a/internal/providers/registry_cache_test.go +++ b/internal/providers/registry_cache_test.go @@ -12,6 +12,7 @@ import ( "github.com/enterpilot/gomodel/config" "github.com/enterpilot/gomodel/internal/cache/modelcache" "github.com/enterpilot/gomodel/internal/core" + "github.com/enterpilot/gomodel/internal/modeldata" ) func TestCacheFile(t *testing.T) { @@ -886,3 +887,40 @@ func TestSaveToCache_SkipsStaleProviderInventory(t *testing.T) { t.Error("recovered provider beta missing from cache, want persisted again") } } + +func TestCacheFile_ModelListETagRoundtrip(t *testing.T) { + tmpDir := t.TempDir() + cacheFile := filepath.Join(tmpDir, "models.json") + + raw := []byte(`{"version": 1, "providers": {}, "models": {"m": {"display_name": "M", "modes": ["chat"]}}, "provider_models": {}}`) + list, err := modeldata.Parse(raw) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + + saving := NewModelRegistry() + saving.SetCache(modelcache.NewLocalCache(cacheFile)) + mock := ®istryMockProvider{ + name: "openai", + modelsResponse: &core.ModelsResponse{ + Object: "list", + Data: []core.Model{{ID: "gpt-4o", Object: "model", OwnedBy: "openai"}}, + }, + } + saving.RegisterProviderWithNameAndType(mock, "openai", "openai") + _ = saving.Initialize(context.Background()) + saving.setModelListAndEnrich(list, raw, `"list-v7"`) + if err := saving.SaveToCache(context.Background()); err != nil { + t.Fatalf("SaveToCache() error = %v", err) + } + + loading := NewModelRegistry() + loading.SetCache(modelcache.NewLocalCache(cacheFile)) + loading.RegisterProviderWithNameAndType(mock, "openai", "openai") + if _, err := loading.LoadFromCache(context.Background()); err != nil { + t.Fatalf("LoadFromCache() error = %v", err) + } + if got := loading.currentModelListETag(); got != `"list-v7"` { + t.Fatalf("currentModelListETag() after load = %q, want %q", got, `"list-v7"`) + } +} diff --git a/internal/providers/registry_init.go b/internal/providers/registry_init.go index 6bfba85c8..c577b6c51 100644 --- a/internal/providers/registry_init.go +++ b/internal/providers/registry_init.go @@ -638,7 +638,9 @@ func (r *ModelRegistry) recheckFailedProviders(ctx context.Context) { } // RefreshModelList fetches the external model metadata list and re-enriches all -// currently registered models. It does not persist the model cache; callers that +// currently registered models. The fetch is conditional: when upstream reports +// the list unchanged for the stored ETag, the current data is kept and its +// model count returned. It does not persist the model cache; callers that // want durable startup data should call SaveToCache after this succeeds. func (r *ModelRegistry) RefreshModelList(ctx context.Context, url string) (int, error) { if strings.TrimSpace(url) == "" { @@ -654,21 +656,27 @@ func (r *ModelRegistry) RefreshModelList(ctx context.Context, url string) (int, } defer release() - models, _, err := r.refreshModelListLocked(ctx, url) + models, _, _, err := r.refreshModelListLocked(ctx, url) return models, err } -func (r *ModelRegistry) refreshModelListLocked(ctx context.Context, url string) (int, metadataEnrichmentStats, error) { - list, raw, err := modeldata.Fetch(ctx, url) +// refreshModelListLocked fetches the model list conditionally: when the stored +// ETag still matches upstream, the download, reparse, and re-enrichment are all +// skipped and changed=false is returned with the current model count. +func (r *ModelRegistry) refreshModelListLocked(ctx context.Context, url string) (int, bool, metadataEnrichmentStats, error) { + result, err := modeldata.FetchIfChanged(ctx, url, r.currentModelListETag()) if err != nil { - return 0, metadataEnrichmentStats{}, err + return 0, false, metadataEnrichmentStats{}, err } - if list == nil { - return 0, metadataEnrichmentStats{}, nil + if result.NotModified { + return r.modelListModelCount(), false, metadataEnrichmentStats{}, nil + } + if result.List == nil { + return 0, false, metadataEnrichmentStats{}, nil } - metadataStats := r.setModelListAndEnrich(list, raw) - return len(list.Models), metadataStats, nil + metadataStats := r.setModelListAndEnrich(result.List, result.Raw, result.ETag) + return len(result.List.Models), true, metadataStats, nil } // refreshModelList fetches the model list and re-enriches all models. @@ -685,11 +693,12 @@ func (r *ModelRegistry) refreshModelList(ctx context.Context, url string) { } var ( models int + changed bool metadataStats metadataEnrichmentStats ) func() { defer release() - models, metadataStats, err = r.refreshModelListLocked(fetchCtx, url) + models, changed, metadataStats, err = r.refreshModelListLocked(fetchCtx, url) }() if err != nil { if !isBenignBackgroundRefreshError(ctx, err) { @@ -697,6 +706,10 @@ func (r *ModelRegistry) refreshModelList(ctx context.Context, url string) { } return } + if !changed { + slog.Debug("model list unchanged", "models", models) + return + } if models == 0 { return } diff --git a/internal/providers/registry_metadata.go b/internal/providers/registry_metadata.go index 8e2139db1..81a6dd086 100644 --- a/internal/providers/registry_metadata.go +++ b/internal/providers/registry_metadata.go @@ -13,12 +13,15 @@ import ( "github.com/enterpilot/gomodel/internal/modeldata" ) -// SetModelList stores the parsed model list and its raw bytes for cache persistence. +// SetModelList stores the parsed model list and its raw bytes for cache +// persistence. The ETag validator is cleared: callers that fetched +// conditionally use setModelListAndEnrich, which records it. func (r *ModelRegistry) SetModelList(list *modeldata.ModelList, raw json.RawMessage) { r.mu.Lock() defer r.mu.Unlock() r.modelList = list r.modelListRaw = raw + r.modelListETag = "" } // EnrichModels re-applies model list metadata to all currently registered models. @@ -60,14 +63,34 @@ func (r *ModelRegistry) enrichModelsLocked() metadataEnrichmentStats { return stats } -func (r *ModelRegistry) setModelListAndEnrich(list *modeldata.ModelList, raw json.RawMessage) metadataEnrichmentStats { +func (r *ModelRegistry) setModelListAndEnrich(list *modeldata.ModelList, raw json.RawMessage, etag string) metadataEnrichmentStats { r.mu.Lock() defer r.mu.Unlock() r.modelList = list r.modelListRaw = raw + r.modelListETag = etag return r.enrichModelsLocked() } +// currentModelListETag returns the model list validator for conditional +// refetches. +func (r *ModelRegistry) currentModelListETag() string { + r.mu.RLock() + defer r.mu.RUnlock() + return r.modelListETag +} + +// modelListModelCount returns the number of models in the currently stored +// model list, or 0 when none is loaded. +func (r *ModelRegistry) modelListModelCount() int { + r.mu.RLock() + defer r.mu.RUnlock() + if r.modelList == nil { + return 0 + } + return len(r.modelList.Models) +} + // ResolveMetadata resolves metadata for a model directly via the stored model list, // bypassing the registry key lookup. This handles cases where the usage DB stores // a response model ID (e.g., "gpt-4o-2024-08-06") that differs from the registry diff --git a/internal/providers/registry_test.go b/internal/providers/registry_test.go index d02fdb5a2..f4cb52a2e 100644 --- a/internal/providers/registry_test.go +++ b/internal/providers/registry_test.go @@ -2564,3 +2564,80 @@ func TestProviderByTypeAndNameTrimConfiguredValues(t *testing.T) { t.Fatalf("GetProviderNameForType(openai) = %q, want %q", got, "padded-name") } } + +func TestRefreshModelList_ConditionalFetch(t *testing.T) { + const etag = `"list-v1"` + body := []byte(`{ + "version": 1, + "updated_at": "2025-01-01T00:00:00Z", + "providers": {"openai": {"display_name": "OpenAI", "api_type": "openai"}}, + "models": {"test-model": {"display_name": "Test Model", "modes": ["chat"]}}, + "provider_models": {} + }`) + + var fullFetches, notModified atomic.Int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("If-None-Match") == etag { + notModified.Add(1) + w.WriteHeader(http.StatusNotModified) + return + } + fullFetches.Add(1) + w.Header().Set("ETag", etag) + _, _ = w.Write(body) + })) + defer server.Close() + + registry := NewModelRegistry() + + count, err := registry.RefreshModelList(context.Background(), server.URL) + if err != nil { + t.Fatalf("RefreshModelList() error = %v", err) + } + if count != 1 { + t.Fatalf("RefreshModelList() count = %d, want 1", count) + } + if fullFetches.Load() != 1 || notModified.Load() != 0 { + t.Fatalf("expected one full fetch, got full=%d notModified=%d", fullFetches.Load(), notModified.Load()) + } + + registry.mu.RLock() + listBefore := registry.modelList + registry.mu.RUnlock() + + count, err = registry.RefreshModelList(context.Background(), server.URL) + if err != nil { + t.Fatalf("RefreshModelList() second call error = %v", err) + } + if count != 1 { + t.Fatalf("RefreshModelList() second call count = %d, want 1", count) + } + if notModified.Load() != 1 { + t.Fatalf("expected second fetch to be answered 304, got full=%d notModified=%d", fullFetches.Load(), notModified.Load()) + } + + registry.mu.RLock() + listAfter := registry.modelList + etagAfter := registry.modelListETag + registry.mu.RUnlock() + if listAfter != listBefore { + t.Fatal("expected 304 refresh to keep the existing parsed model list") + } + if etagAfter != etag { + t.Fatalf("modelListETag = %q, want %q", etagAfter, etag) + } +} + +func TestSetModelList_ClearsETag(t *testing.T) { + registry := NewModelRegistry() + raw := []byte(`{"version": 1, "providers": {}, "models": {}, "provider_models": {}}`) + list, err := modeldata.Parse(raw) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + registry.setModelListAndEnrich(list, raw, `"old"`) + registry.SetModelList(list, raw) + if got := registry.currentModelListETag(); got != "" { + t.Fatalf("currentModelListETag() = %q, want empty after SetModelList", got) + } +} From 127695175bad1497b0f71d8ae4ed862c2102baaf Mon Sep 17 00:00:00 2001 From: "Jakub A. W" Date: Mon, 24 Aug 2026 16:10:52 +0200 Subject: [PATCH 2/2] fix(models): scope the model list ETag to its source URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups: an HTTP validator identifies one representation of one resource, so the stored ETag is now recorded with the URL that issued it (persisted in the model cache as well) and never presented to a reconfigured MODEL_LIST_URL — some servers derive ETags from mtime+size rather than content, where a cross-URL match would wrongly 304 and pin a stale catalog. A 304 response's own ETag now refreshes the stored validator per RFC 9111, and StartBackgroundRefresh trims the URL so whitespace disables scheduling like it disables fetching. --- internal/cache/modelcache/modelcache.go | 3 + internal/modeldata/fetcher.go | 6 ++ internal/modeldata/fetcher_test.go | 19 ++++++ internal/providers/init.go | 5 +- internal/providers/registry.go | 7 +- internal/providers/registry_cache.go | 4 +- internal/providers/registry_cache_test.go | 7 +- internal/providers/registry_init.go | 9 ++- internal/providers/registry_metadata.go | 32 +++++++-- internal/providers/registry_test.go | 82 ++++++++++++++++++++++- 10 files changed, 159 insertions(+), 15 deletions(-) diff --git a/internal/cache/modelcache/modelcache.go b/internal/cache/modelcache/modelcache.go index 472d6a466..9004e9908 100644 --- a/internal/cache/modelcache/modelcache.go +++ b/internal/cache/modelcache/modelcache.go @@ -21,7 +21,10 @@ type ModelCache struct { ModelListData json.RawMessage `json:"model_list_data,omitempty"` // ModelListETag is the HTTP validator ModelListData was downloaded with, // letting the next fetch skip the download when the list is unchanged. + // ModelListURL records which URL issued it, so the validator is never + // presented to a reconfigured model list URL. ModelListETag string `json:"model_list_etag,omitempty"` + ModelListURL string `json:"model_list_url,omitempty"` } // CachedProvider holds shared fields for all models from a single provider. diff --git a/internal/modeldata/fetcher.go b/internal/modeldata/fetcher.go index ba2fc4add..347d83014 100644 --- a/internal/modeldata/fetcher.go +++ b/internal/modeldata/fetcher.go @@ -67,6 +67,12 @@ func FetchIfChanged(ctx context.Context, url, etag string) (FetchResult, error) defer resp.Body.Close() if etag != "" && resp.StatusCode == http.StatusNotModified { + // RFC 9111: a 304 may carry updated metadata for the stored + // representation; adopt its ETag when present so future conditional + // requests use the server's current validator. + if respETag := resp.Header.Get("ETag"); respETag != "" { + etag = respETag + } return FetchResult{ETag: etag, NotModified: true}, nil } if resp.StatusCode != http.StatusOK { diff --git a/internal/modeldata/fetcher_test.go b/internal/modeldata/fetcher_test.go index bf1807b2b..72feade70 100644 --- a/internal/modeldata/fetcher_test.go +++ b/internal/modeldata/fetcher_test.go @@ -163,6 +163,25 @@ func TestFetchIfChanged_NotModified(t *testing.T) { } } +func TestFetchIfChanged_NotModifiedAdoptsResponseETag(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"refreshed"`) + w.WriteHeader(http.StatusNotModified) + })) + defer server.Close() + + result, err := FetchIfChanged(context.Background(), server.URL, `"stale"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !result.NotModified { + t.Fatal("expected NotModified=true for 304 response") + } + if result.ETag != `"refreshed"` { + t.Errorf("ETag = %q, want the 304's refreshed validator", result.ETag) + } +} + func TestFetchIfChanged_ChangedContent(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("ETag", `"v2"`) diff --git a/internal/providers/init.go b/internal/providers/init.go index 1ea94f21f..725c905b5 100644 --- a/internal/providers/init.go +++ b/internal/providers/init.go @@ -133,12 +133,13 @@ func Init(ctx context.Context, result *config.LoadResult, factory *ProviderFacto fetchCtx, cancel := context.WithTimeout(ctx, 45*time.Second) defer cancel() - result, err := modeldata.FetchIfChanged(fetchCtx, modelListURL, registry.currentModelListETag()) + result, err := modeldata.FetchIfChanged(fetchCtx, modelListURL, registry.currentModelListETag(modelListURL)) if err != nil { slog.Warn("failed to fetch model list", "url", modelListURL, "error", err) return } if result.NotModified { + registry.updateModelListValidator(result.ETag, modelListURL) slog.Info("model list unchanged since last download, using cached copy") return } @@ -146,7 +147,7 @@ func Init(ctx context.Context, result *config.LoadResult, factory *ProviderFacto return } - metadataStats := registry.setModelListAndEnrich(result.List, result.Raw, result.ETag) + metadataStats := registry.setModelListAndEnrich(result.List, result.Raw, result.ETag, modelListURL) if err := registry.SaveToCache(fetchCtx); err != nil { slog.Warn("failed to save cache after model list fetch", "error", err) diff --git a/internal/providers/registry.go b/internal/providers/registry.go index e71dce178..929632085 100644 --- a/internal/providers/registry.go +++ b/internal/providers/registry.go @@ -62,7 +62,12 @@ type ModelRegistry struct { refreshOnce sync.Once // initializes refreshCh for zero-value safety modelList *modeldata.ModelList // parsed model list (nil = not loaded) modelListRaw json.RawMessage // raw bytes for cache persistence - modelListETag string // validator for conditional refetches; empty = fetch unconditionally + // modelListETag is the validator for conditional refetches and + // modelListETagURL the URL it was issued by; the validator is only sent + // back to that same URL, so a reconfigured MODEL_LIST_URL always fetches + // unconditionally. Empty etag = fetch unconditionally. + modelListETag string + modelListETagURL string // configMetadataOverrides holds operator-supplied metadata keyed by provider // instance name -> raw model ID. Applied after remote-registry enrichment as // a higher-priority layer. nil if no overrides declared. diff --git a/internal/providers/registry_cache.go b/internal/providers/registry_cache.go index da8dc4840..a318f61b8 100644 --- a/internal/providers/registry_cache.go +++ b/internal/providers/registry_cache.go @@ -134,7 +134,7 @@ func (r *ModelRegistry) LoadFromCache(ctx context.Context) (int, error) { if list != nil { r.modelList = list r.modelListRaw = modelCache.ModelListData - r.modelListETag = modelCache.ModelListETag + r.setModelListValidatorLocked(modelCache.ModelListETag, modelCache.ModelListURL) } r.mu.Unlock() @@ -168,6 +168,7 @@ func (r *ModelRegistry) SaveToCache(ctx context.Context) error { maps.Copy(providerTypes, r.providerTypes) modelListRaw := r.modelListRaw modelListETag := r.modelListETag + modelListETagURL := r.modelListETagURL r.mu.RUnlock() if cacheBackend == nil { @@ -179,6 +180,7 @@ func (r *ModelRegistry) SaveToCache(ctx context.Context) error { Providers: make(map[string]modelcache.CachedProvider, len(modelsByProvider)), ModelListData: modelListRaw, ModelListETag: modelListETag, + ModelListURL: modelListETagURL, } var totalModels int diff --git a/internal/providers/registry_cache_test.go b/internal/providers/registry_cache_test.go index d58dbd2be..456aa7727 100644 --- a/internal/providers/registry_cache_test.go +++ b/internal/providers/registry_cache_test.go @@ -909,7 +909,7 @@ func TestCacheFile_ModelListETagRoundtrip(t *testing.T) { } saving.RegisterProviderWithNameAndType(mock, "openai", "openai") _ = saving.Initialize(context.Background()) - saving.setModelListAndEnrich(list, raw, `"list-v7"`) + saving.setModelListAndEnrich(list, raw, `"list-v7"`, "https://example.test/models.min.json") if err := saving.SaveToCache(context.Background()); err != nil { t.Fatalf("SaveToCache() error = %v", err) } @@ -920,7 +920,10 @@ func TestCacheFile_ModelListETagRoundtrip(t *testing.T) { if _, err := loading.LoadFromCache(context.Background()); err != nil { t.Fatalf("LoadFromCache() error = %v", err) } - if got := loading.currentModelListETag(); got != `"list-v7"` { + if got := loading.currentModelListETag("https://example.test/models.min.json"); got != `"list-v7"` { t.Fatalf("currentModelListETag() after load = %q, want %q", got, `"list-v7"`) } + if got := loading.currentModelListETag("https://other.test/models.min.json"); got != "" { + t.Fatalf("currentModelListETag() for another URL = %q, want empty", got) + } } diff --git a/internal/providers/registry_init.go b/internal/providers/registry_init.go index c577b6c51..26f196f92 100644 --- a/internal/providers/registry_init.go +++ b/internal/providers/registry_init.go @@ -554,6 +554,9 @@ func (r *ModelRegistry) IsInitialized() bool { // for the goroutine to exit before returning, so callers should expect it to // block during shutdown until any in-flight refresh work unwinds. func (r *ModelRegistry) StartBackgroundRefresh(interval, recheckInterval time.Duration, modelListURL string) func() { + // Normalize once so a whitespace-only URL disables model list refreshes + // here just like it does on the direct RefreshModelList path. + modelListURL = strings.TrimSpace(modelListURL) if interval <= 0 { // time.NewTicker panics on non-positive durations and a refresh loop // with a zero interval would be meaningless. Skip the goroutine and @@ -664,18 +667,20 @@ func (r *ModelRegistry) RefreshModelList(ctx context.Context, url string) (int, // ETag still matches upstream, the download, reparse, and re-enrichment are all // skipped and changed=false is returned with the current model count. func (r *ModelRegistry) refreshModelListLocked(ctx context.Context, url string) (int, bool, metadataEnrichmentStats, error) { - result, err := modeldata.FetchIfChanged(ctx, url, r.currentModelListETag()) + result, err := modeldata.FetchIfChanged(ctx, url, r.currentModelListETag(url)) if err != nil { return 0, false, metadataEnrichmentStats{}, err } if result.NotModified { + // A 304 may carry a refreshed validator for the unchanged content. + r.updateModelListValidator(result.ETag, url) return r.modelListModelCount(), false, metadataEnrichmentStats{}, nil } if result.List == nil { return 0, false, metadataEnrichmentStats{}, nil } - metadataStats := r.setModelListAndEnrich(result.List, result.Raw, result.ETag) + metadataStats := r.setModelListAndEnrich(result.List, result.Raw, result.ETag, url) return len(result.List.Models), true, metadataStats, nil } diff --git a/internal/providers/registry_metadata.go b/internal/providers/registry_metadata.go index 81a6dd086..68a655d41 100644 --- a/internal/providers/registry_metadata.go +++ b/internal/providers/registry_metadata.go @@ -22,6 +22,7 @@ func (r *ModelRegistry) SetModelList(list *modeldata.ModelList, raw json.RawMess r.modelList = list r.modelListRaw = raw r.modelListETag = "" + r.modelListETagURL = "" } // EnrichModels re-applies model list metadata to all currently registered models. @@ -63,23 +64,44 @@ func (r *ModelRegistry) enrichModelsLocked() metadataEnrichmentStats { return stats } -func (r *ModelRegistry) setModelListAndEnrich(list *modeldata.ModelList, raw json.RawMessage, etag string) metadataEnrichmentStats { +func (r *ModelRegistry) setModelListAndEnrich(list *modeldata.ModelList, raw json.RawMessage, etag, url string) metadataEnrichmentStats { r.mu.Lock() defer r.mu.Unlock() r.modelList = list r.modelListRaw = raw - r.modelListETag = etag + r.setModelListValidatorLocked(etag, url) return r.enrichModelsLocked() } -// currentModelListETag returns the model list validator for conditional -// refetches. -func (r *ModelRegistry) currentModelListETag() string { +func (r *ModelRegistry) setModelListValidatorLocked(etag, url string) { + if etag == "" { + url = "" + } + r.modelListETag = etag + r.modelListETagURL = url +} + +// currentModelListETag returns the model list validator for a conditional +// refetch of url, or empty when the stored validator was issued by a +// different URL — validators identify one representation of one resource. +func (r *ModelRegistry) currentModelListETag(url string) string { r.mu.RLock() defer r.mu.RUnlock() + if r.modelListETagURL != url { + return "" + } return r.modelListETag } +// updateModelListValidator records the validator a 304 response carried for +// url, keeping the stored model list as-is. Per RFC 9111 a 304 may refresh the +// stored ETag. +func (r *ModelRegistry) updateModelListValidator(etag, url string) { + r.mu.Lock() + defer r.mu.Unlock() + r.setModelListValidatorLocked(etag, url) +} + // modelListModelCount returns the number of models in the currently stored // model list, or 0 when none is loaded. func (r *ModelRegistry) modelListModelCount() int { diff --git a/internal/providers/registry_test.go b/internal/providers/registry_test.go index f4cb52a2e..26adaef0b 100644 --- a/internal/providers/registry_test.go +++ b/internal/providers/registry_test.go @@ -2628,6 +2628,84 @@ func TestRefreshModelList_ConditionalFetch(t *testing.T) { } } +func TestRefreshModelList_ETagNotSentToDifferentURL(t *testing.T) { + const etag = `"list-v1"` + body := []byte(`{ + "version": 1, + "updated_at": "2025-01-01T00:00:00Z", + "providers": {}, + "models": {"test-model": {"display_name": "Test Model", "modes": ["chat"]}}, + "provider_models": {} + }`) + + newServer := func(counter *atomic.Int64) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("If-None-Match") != "" { + counter.Add(1) + } + w.Header().Set("ETag", etag) + _, _ = w.Write(body) + })) + } + + var firstConditional, secondConditional atomic.Int64 + first := newServer(&firstConditional) + defer first.Close() + second := newServer(&secondConditional) + defer second.Close() + + registry := NewModelRegistry() + if _, err := registry.RefreshModelList(context.Background(), first.URL); err != nil { + t.Fatalf("RefreshModelList() error = %v", err) + } + if _, err := registry.RefreshModelList(context.Background(), second.URL); err != nil { + t.Fatalf("RefreshModelList() against second URL error = %v", err) + } + if secondConditional.Load() != 0 { + t.Fatal("expected no If-None-Match against a different URL: validators identify one resource") + } + if got := registry.currentModelListETag(second.URL); got != etag { + t.Fatalf("currentModelListETag(second) = %q, want %q", got, etag) + } + if got := registry.currentModelListETag(first.URL); got != "" { + t.Fatalf("currentModelListETag(first) = %q, want empty after refreshing from second URL", got) + } +} + +func TestRefreshModelList_304AdoptsRefreshedETag(t *testing.T) { + body := []byte(`{ + "version": 1, + "updated_at": "2025-01-01T00:00:00Z", + "providers": {}, + "models": {"test-model": {"display_name": "Test Model", "modes": ["chat"]}}, + "provider_models": {} + }`) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("If-None-Match") != "" { + // Same content, refreshed validator: RFC 9111 lets a 304 update + // the stored ETag. + w.Header().Set("ETag", `"list-v2"`) + w.WriteHeader(http.StatusNotModified) + return + } + w.Header().Set("ETag", `"list-v1"`) + _, _ = w.Write(body) + })) + defer server.Close() + + registry := NewModelRegistry() + if _, err := registry.RefreshModelList(context.Background(), server.URL); err != nil { + t.Fatalf("RefreshModelList() error = %v", err) + } + if _, err := registry.RefreshModelList(context.Background(), server.URL); err != nil { + t.Fatalf("RefreshModelList() second call error = %v", err) + } + if got := registry.currentModelListETag(server.URL); got != `"list-v2"` { + t.Fatalf("currentModelListETag() = %q, want refreshed %q", got, `"list-v2"`) + } +} + func TestSetModelList_ClearsETag(t *testing.T) { registry := NewModelRegistry() raw := []byte(`{"version": 1, "providers": {}, "models": {}, "provider_models": {}}`) @@ -2635,9 +2713,9 @@ func TestSetModelList_ClearsETag(t *testing.T) { if err != nil { t.Fatalf("Parse() error = %v", err) } - registry.setModelListAndEnrich(list, raw, `"old"`) + registry.setModelListAndEnrich(list, raw, `"old"`, "https://example.test/models.min.json") registry.SetModelList(list, raw) - if got := registry.currentModelListETag(); got != "" { + if got := registry.currentModelListETag("https://example.test/models.min.json"); got != "" { t.Fatalf("currentModelListETag() = %q, want empty after SetModelList", got) } }