From 4c4c06aa359fe96cabd5efc98c3aa0b94981a7fb Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 26 Aug 2026 17:57:31 -0700 Subject: [PATCH] fix: honor fresh provider catalog discovery (#130412) * fix: honor fresh provider catalog discovery * test: use compatible deferred catalog fixture --- .../src/external-server/discovery.test.ts | 63 ++++++++++++---- .../provider-catalog-shared.test.ts | 72 +++++++++++++++++++ src/plugin-sdk/provider-catalog-shared.ts | 37 +++++----- 3 files changed, 140 insertions(+), 32 deletions(-) diff --git a/extensions/llama-cpp/src/external-server/discovery.test.ts b/extensions/llama-cpp/src/external-server/discovery.test.ts index 6569f592f35e..77b63df58e8b 100644 --- a/extensions/llama-cpp/src/external-server/discovery.test.ts +++ b/extensions/llama-cpp/src/external-server/discovery.test.ts @@ -1,3 +1,5 @@ +import { once } from "node:events"; +import { createServer } from "node:http"; import { clearLiveCatalogCacheForTests } from "openclaw/plugin-sdk/provider-catalog-shared"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { discoverLlamaServer } from "./discovery.js"; @@ -88,21 +90,56 @@ describe("llama-server discovery projection", () => { }); }); - it("bypasses the shared cache for credential-scoped discovery", async () => { - discoverRowsMock.mockResolvedValue({ - kind: "success", - health: "ready", - fetchedAt: 123, - rows: [], + it.each([ + { name: "API key", access: { apiKey: "endpoint-key" } }, + { name: "authorization header", access: { headers: { Authorization: "Bearer endpoint-key" } } }, + { name: "explicit refresh", access: { cacheTtlMs: 0 } }, + ])("fetches $name discovery after an anonymous catalog was cached", async ({ access }) => { + const actual = await vi.importActual( + "openclaw/plugin-sdk/provider-setup", + ); + discoverRowsMock.mockImplementation(actual.discoverOpenAICompatibleLocalModels); + let modelId = "anonymous-model"; + const modelRequests: Array = []; + const server = createServer((request, response) => { + response.setHeader("Content-Type", "application/json"); + if (request.url === "/models") { + modelRequests.push(request.headers.authorization); + response.end(JSON.stringify({ data: [{ id: modelId, status: { value: "unloaded" } }] })); + } else { + response.end("{}"); + } }); - - for (let index = 0; index < 2; index += 1) { - await discoverLlamaServer({ - baseUrl: "http://localhost:8080", - headers: { Authorization: "Bearer endpoint-key" }, + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + try { + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("expected a listening TCP server"); + } + const baseUrl = `http://127.0.0.1:${address.port}`; + await expect(discoverLlamaServer({ baseUrl })).resolves.toMatchObject({ + kind: "success", + models: [{ config: { id: "anonymous-model" } }], + }); + modelId = "fresh-model"; + await expect(discoverLlamaServer({ baseUrl, ...access })).resolves.toMatchObject({ + kind: "success", + models: [{ config: { id: "fresh-model" } }], + }); + await expect(discoverLlamaServer({ baseUrl })).resolves.toMatchObject({ + kind: "success", + models: [{ config: { id: "anonymous-model" } }], + }); + expect(modelRequests).toEqual([ + undefined, + "cacheTtlMs" in access ? undefined : "Bearer endpoint-key", + ]); + } finally { + server.closeAllConnections(); + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); }); } - - expect(discoverRowsMock).toHaveBeenCalledTimes(2); }); }); diff --git a/src/plugin-sdk/provider-catalog-shared.test.ts b/src/plugin-sdk/provider-catalog-shared.test.ts index 3f4acc4997fc..7dafc6032d65 100644 --- a/src/plugin-sdk/provider-catalog-shared.test.ts +++ b/src/plugin-sdk/provider-catalog-shared.test.ts @@ -1,6 +1,7 @@ // Provider catalog shared tests cover catalog hashing, normalization, and model visibility. import type { ModelCatalogProvider } from "@openclaw/model-catalog-core/model-catalog-types"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createDeferred } from "../../test/helpers/promise.js"; import { applyProviderNativeStreamingUsageCompat, buildManifestModelDefinition, @@ -94,6 +95,77 @@ describe("provider-catalog-shared live catalog cache", () => { expect(load).toHaveBeenCalledTimes(2); }); + it.each(["resolve", "reject", "throw"] as const)( + "bypasses a warm cache without modifying it when the uncached loader will %s", + async (outcome) => { + const keyParts = ["provider", "models"]; + await getCachedLiveCatalogValue({ keyParts, load: async () => "cached" }); + const error = new Error("uncached failure"); + const load = vi.fn(() => { + if (outcome === "throw") { + throw error; + } + return outcome === "reject" ? Promise.reject(error) : Promise.resolve("fresh"); + }); + const shouldCache = vi.fn(() => false); + const fresh = getCachedLiveCatalogValue({ keyParts, load, shouldCache, ttlMs: 0 }); + if (outcome === "resolve") { + await expect(fresh).resolves.toBe("fresh"); + } else { + await expect(fresh).rejects.toBe(error); + } + expect(shouldCache).not.toHaveBeenCalled(); + await expect(getCachedLiveCatalogValue({ keyParts, load })).resolves.toBe("cached"); + expect(load).toHaveBeenCalledTimes(1); + }, + ); + + it.each(["reject", "predicate-false", "predicate-throw", "same-promise"] as const)( + "preserves a replacement cache entry after expired work finishes with %s", + async (outcome) => { + let now = 1_000; + const keyParts = ["provider", "models"]; + const pending = createDeferred(); + const error = new Error("expired failure"); + const first = getCachedLiveCatalogValue({ + keyParts, + load: () => pending.promise, + ttlMs: 100, + now: () => now, + shouldCache: () => { + if (outcome === "predicate-throw") { + throw error; + } + return false; + }, + }); + now = 1_101; + const replacement = getCachedLiveCatalogValue({ + keyParts, + load: () => (outcome === "same-promise" ? pending.promise : Promise.resolve("replacement")), + ttlMs: 100, + now: () => now, + }); + if (outcome === "reject") { + pending.reject(error); + } else { + pending.resolve("expired"); + } + if (outcome === "reject" || outcome === "predicate-throw") { + await expect(first).rejects.toBe(error); + } else { + await expect(first).resolves.toBe("expired"); + } + const expected = outcome === "same-promise" ? "expired" : "replacement"; + await expect(replacement).resolves.toBe(expected); + const load = vi.fn(async () => "unnecessary reload"); + await expect(getCachedLiveCatalogValue({ keyParts, load, now: () => now })).resolves.toBe( + expected, + ); + expect(load).not.toHaveBeenCalled(); + }, + ); + it("does not retain resolved live catalog values rejected by the cache predicate", async () => { const load = vi .fn<() => Promise>() diff --git a/src/plugin-sdk/provider-catalog-shared.ts b/src/plugin-sdk/provider-catalog-shared.ts index 1844d24115ff..efc395b3719f 100644 --- a/src/plugin-sdk/provider-catalog-shared.ts +++ b/src/plugin-sdk/provider-catalog-shared.ts @@ -80,7 +80,11 @@ export async function getCachedLiveCatalogValue(params: { now?: () => number; }): Promise { const rawNow = params.now?.() ?? Date.now(); - const ttlMs = params.ttlMs ?? 30_000; + const expiresAt = resolveExpiresAtMsFromDurationMs(params.ttlMs ?? 30_000, { nowMs: rawNow }); + // Uncached callers must neither reuse nor disturb an existing entry. + if (expiresAt === undefined) { + return await params.load(); + } const key = buildLiveCatalogCacheKey(params.keyParts); const existing = liveCatalogCache.get(key) as LiveCatalogCacheEntry | undefined; if (existing) { @@ -89,27 +93,22 @@ export async function getCachedLiveCatalogValue(params: { } liveCatalogCache.delete(key); } - const value = params.load(); - const expiresAt = resolveExpiresAtMsFromDurationMs(ttlMs, { nowMs: rawNow }); - if (expiresAt !== undefined) { - // Auth-scoped live provider catalogs can vary by token; keep this - // process-local cache bounded so discovery cannot grow without limit. - pruneMapToMaxSize(liveCatalogCache, LIVE_CATALOG_CACHE_MAX_ENTRIES - 1); - liveCatalogCache.set(key, { - expiresAt, - value, - }); - } + const entry = { expiresAt, value: params.load() }; + // Auth-scoped live provider catalogs can vary by token; keep this + // process-local cache bounded so discovery cannot grow without limit. + pruneMapToMaxSize(liveCatalogCache, LIVE_CATALOG_CACHE_MAX_ENTRIES - 1); + liveCatalogCache.set(key, entry); + let retain = false; try { - const resolved = await value; - if (params.shouldCache && !params.shouldCache(resolved)) { + const resolved = await entry.value; + retain = params.shouldCache?.(resolved) ?? true; + return resolved; + } finally { + // Expired work may finish after a replacement load. Only its own entry + // can be removed when loading or the cache predicate fails. + if (!retain && liveCatalogCache.get(key) === entry) { liveCatalogCache.delete(key); } - return resolved; - } catch (err) { - // Failed live discovery should not poison later retries for the same provider/config. - liveCatalogCache.delete(key); - throw err; } }