diff --git a/changelog.d/fixes/8728-model-catalog-swr.md b/changelog.d/fixes/8728-model-catalog-swr.md new file mode 100644 index 0000000000..9272b3f0b7 --- /dev/null +++ b/changelog.d/fixes/8728-model-catalog-swr.md @@ -0,0 +1 @@ +- **fix(api):** make `/v1/models` stale refresh response-safe and generation-safe, with narrow synced-model invalidation ([#8728](https://github.com/diegosouzapw/OmniRoute/pull/8728)). Related to #8697. diff --git a/src/app/api/v1/models/catalog.ts b/src/app/api/v1/models/catalog.ts index 34ccbdfbaf..a16590fb7e 100644 --- a/src/app/api/v1/models/catalog.ts +++ b/src/app/api/v1/models/catalog.ts @@ -105,18 +105,25 @@ export { getCustomVisionCapabilityFields }; // lives in ./catalogCache. Re-exported here because the existing tests import the // hooks from this module, and CATALOG_STALE_WHILE_REVALIDATE_MS is part of the // documented behavior of this endpoint. -import { CATALOG_CACHE_TTL_MS_DEFAULT, resolveCachedCatalogResponse } from "./catalogCache"; +import { + CATALOG_CACHE_TTL_MS_DEFAULT, + resolveCachedCatalogResponse, + type CatalogCachePolicy, +} from "./catalogCache"; export { CATALOG_STALE_WHILE_REVALIDATE_MS, + getCatalogStaleWhileRevalidateMs, __resetCatalogBuilderRunsForTest, __getCatalogBuilderRunsForTest, __expireCatalogCacheForTest, __setCatalogCacheEntryForTest, __flushCatalogBackgroundRefreshForTest, __forceCatalogInFlightRejectionForTest, + __setCatalogStaleWhileRevalidateAccessorForTest, + __setCatalogStaleWhileRevalidateMsForTest, } from "./catalogCache"; -export type { CachedCatalog } from "./catalogCache"; +export type { CachedCatalog, CatalogCachePolicy } from "./catalogCache"; /** * Build unified OpenAI-compatible model catalog response. @@ -124,7 +131,8 @@ export type { CachedCatalog } from "./catalogCache"; */ export async function getUnifiedModelsResponse( request: Request, - corsHeaders: Record = {} + corsHeaders: Record = {}, + cachePolicy: CatalogCachePolicy = {} ) { const diagnosticHeaders = getCatalogDiagnosticsHeaders({ request }); @@ -156,7 +164,8 @@ export async function getUnifiedModelsResponse( return await resolveCachedCatalogResponse( request, { corsHeaders, diagnosticHeaders }, - buildCatalogPayload + buildCatalogPayload, + cachePolicy ); } catch (err) { // Hard rule #12: never put a raw err.message/err.stack in a response body. diff --git a/src/app/api/v1/models/catalogCache.ts b/src/app/api/v1/models/catalogCache.ts index 316120293c..da08307bd5 100644 --- a/src/app/api/v1/models/catalogCache.ts +++ b/src/app/api/v1/models/catalogCache.ts @@ -5,15 +5,16 @@ * builder walks 8 registries and hits SQLite for connections, combos, custom * models and aliases; under Next.js's single-threaded App Router request * handling, N concurrent calls execute back-to-back and the Nth completes at - * N × single-request latency. So identical concurrent requests are coalesced - * onto one in-flight promise and the serialized body is memoized for a short - * window. + * N × single-request latency. Identical concurrent requests are therefore + * coalesced onto one in-flight promise and successful serialized bodies are + * memoized for a short fresh window. * * Auth rejection is NOT handled here and must stay in the caller: it depends on * live per-request state (dashboard cookie, API key) and must never be cached. */ import { getModelCatalogCacheVersion } from "@/lib/db/readCache"; import { extractApiKey } from "@/sse/services/auth"; +import { after } from "next/server"; import { isCodexModelCatalogClient } from "./catalogRequest"; @@ -24,7 +25,7 @@ export type CachedCatalog = { expiresAt: number; }; -/** Payload shape returned by the builder the caller injects. */ +/** Payload shape returned by the shared builder primitive the caller injects. */ export type CatalogPayload = { body: string; headers: Record; @@ -32,44 +33,65 @@ export type CatalogPayload = { cacheTTL: number; }; +export type CatalogRefreshTask = () => Promise; +export type CatalogRefreshScheduler = (task: CatalogRefreshTask) => void; + /** - * A client with a short discovery timeout (Claude Code allows 3 s) must never - * wait on a full rebuild. Once a cached 200 expires it is still served - * immediately for up to this long while a background refresh repopulates it. - * Bounded so a refresh that keeps failing cannot pin an old catalog forever — - * past this window callers fall back to waiting, same as a cold cache. + * Per-call cache policy. Request-context routes inject Next.js `after()` as the + * scheduler; unit tests and direct non-framework callers can inject a deterministic + * scheduler without making the cache branch on runner-specific environment variables. */ -export const CATALOG_STALE_WHILE_REVALIDATE_MS = 30_000; +export type CatalogCachePolicy = { + getStaleWhileRevalidateMs?: () => number; + scheduleBackgroundRefresh?: CatalogRefreshScheduler; +}; + +/** + * Production stale-while-revalidate window. + * + * A successful snapshot remains eligible indefinitely after the 60-second fresh TTL. + * TTL expiry requests return that last success and schedule one refresh. Database state + * changes are different: the version signal below hard-invalidates every snapshot and + * makes the next request await a current-generation build. + */ +export const CATALOG_STALE_WHILE_REVALIDATE_MS = Number.POSITIVE_INFINITY; /** * Fallback memoization window; overridden by `settings.cache.modelCatalogCacheTtlMs`. * - * This does NOT govern post-write freshness — `invalidateDbCache()` bumps - * `modelCatalogCacheVersion` on every settings/connections/combos/pricing write and - * `dropCatalogCacheIfStateChanged()` drops the whole cache the moment it moves, so a - * write is reflected on the very next read regardless of this value. What it governs is - * the "nothing was written" case, where replaying a body built seconds ago is precisely - * the point of the cache. - * - * It was 1500 ms, which was shorter than a single build: measured 2026-07-28 on the - * production VPS, the builder takes ~49 s for a 1.3 MB / 2645-model catalog. Any two - * requests more than 1.5 s apart therefore both missed the fresh window, and the second - * fell into stale-while-revalidate — which rebuilds via `setTimeout(…, 0)` and, because - * the builder is overwhelmingly synchronous under the single-threaded App Router, pins - * the event loop so even the "served immediately" stale body only reaches the client - * once the rebuild finishes. Net effect: ~50 s on essentially every call. - * - * Held at 60 s to match the ceiling the settings schema already allows for the override - * (`settingsSchemas.ts`, `.max(60000)`), so the default can never exceed what an - * operator is permitted to configure. + * This is only the fresh window. Ordinary expiry serves the last successful snapshot + * while refreshing. `modelCatalogCacheVersion` changes bypass stale serving entirely. */ export const CATALOG_CACHE_TTL_MS_DEFAULT = 60_000; -const catalogCache = new Map(); -const catalogInFlight = new Map>(); +type CatalogInFlight = { + generation: number; + promise: Promise; +}; +const catalogCache = new Map(); +const catalogInFlight = new Map(); + +let catalogGeneration = 0; +let lastSeenCatalogCacheVersion = getModelCatalogCacheVersion(); +let staleWhileRevalidateMsAccessor = () => CATALOG_STALE_WHILE_REVALIDATE_MS; let _catalogBuilderRuns = 0; +function defaultBackgroundRefreshScheduler(task: CatalogRefreshTask): void { + // All production routes run in Next.js request context, including callers that transform the + // shared response. Direct test/startup callers have no request store and need a safe fallback. + try { + after(task); + } catch { + setImmediate(() => void task()); + } +} + +/** Current SWR policy value; production defaults to unbounded stale serving. */ +export function getCatalogStaleWhileRevalidateMs(): number { + return staleWhileRevalidateMsAccessor(); +} + function buildCatalogCacheKey(request: Request): string { const url = new URL(request.url); const prefix = url.searchParams.get("prefix") || ""; @@ -79,30 +101,28 @@ function buildCatalogCacheKey(request: Request): string { return `${prefix}|${isCodex}|${apiKey}|${configuredOnly}`; } -// Tracks the model-catalog cache version (src/lib/db/readCache.ts) as of the last -// cache access. invalidateDbCache() bumps that version on every settings/connections/ -// combos/pricing write; when it moves on, every memoized entry here was built from -// state that no longer holds, so drop them all rather than keying by version (which -// would leak one Map entry per version forever instead of ever pruning old ones). -let lastSeenCatalogCacheVersion = getModelCatalogCacheVersion(); -function dropCatalogCacheIfStateChanged(): void { +/** + * Observe the DB-side invalidation signal. + * + * Every observed version transition is hard invalidation: snapshots are cleared, + * the local generation advances, and old work is detached. Completion guards also + * call this function, so a version change that occurs while a builder is running + * prevents that builder from writing even before another request arrives. + */ +function synchronizeCatalogGeneration(): void { const currentVersion = getModelCatalogCacheVersion(); if (currentVersion === lastSeenCatalogCacheVersion) return; + lastSeenCatalogCacheVersion = currentVersion; + catalogGeneration++; catalogCache.clear(); - // Deliberately NOT clearing catalogInFlight: an in-flight build already reads live - // DB/settings state as of when it started, so letting it finish and populate the - // (now-current) cache entry is correct — clearing it would just force a redundant - // second builder run for requests that arrive mid-flight. + catalogInFlight.clear(); } // Header sources mix Title-Case keys (diagnostic/cors headers built by app code) with -// lower-case ones (payload headers captured via the Fetch `Headers` iterator). A plain -// object spread keeps both casings as distinct keys, and the `Response` constructor -// then *appends* rather than overwrites them, producing comma-joined duplicates (e.g. -// request-id echoing "foo, foo"). Merge through a real `Headers` so `.set()` overwrites -// case-insensitively. Earlier sources are the base; the caller passes diagnostics last -// so per-request fields reflect the current request, not whichever one filled the cache. +// lower-case ones (payload headers captured via the Fetch `Headers` iterator). Merge +// through a real Headers so the caller's per-request diagnostics overwrite cached values +// case-insensitively. export function mergeCatalogHeaders( ...sources: Array | undefined> ): Headers { @@ -116,66 +136,26 @@ export function mergeCatalogHeaders( return merged; } -function storePayload(cacheKey: string, payload: CatalogPayload): CachedCatalog { - const entry: CachedCatalog = { +function isSuccessfulPayload(payload: CatalogPayload): boolean { + return payload.status >= 200 && payload.status < 300; +} + +function storeSuccessfulPayload( + cacheKey: string, + payload: CatalogPayload, + inFlight: CatalogInFlight +): void { + synchronizeCatalogGeneration(); + if (!isSuccessfulPayload(payload)) return; + if (inFlight.generation !== catalogGeneration) return; + if (catalogInFlight.get(cacheKey) !== inFlight) return; + + catalogCache.set(cacheKey, { body: payload.body, headers: payload.headers, status: payload.status, expiresAt: Date.now() + payload.cacheTTL, - }; - catalogCache.set(cacheKey, entry); - return entry; -} - -/** - * Kick off a background rebuild so an expired-but-stale-eligible entry can be - * refreshed without the current request waiting on it. Reuses catalogInFlight — - * no second coalescing mechanism — so a concurrent cold/stale request for the - * same key joins this refresh instead of starting another. - * - * The builder runs one macrotask later so the stale response that triggered this - * call is handed back before the builder's synchronous prologue runs; the whole - * point of this path is that the caller does not pay for the rebuild. - * - * The tracked promise **rejects** on failure. catalogInFlight is shared with the - * cold path: a caller whose entry aged past the stale window skips the stale - * branch and awaits whatever promise it finds here, and resolving with the stale - * entry would hand it a body it was no longer entitled to while disguising a - * build failure as a 200. The rejection is pre-handled so this path can never - * raise an unhandledRejection; a failed refresh simply never overwrites the entry. - */ -function scheduleBackgroundRefresh( - cacheKey: string, - request: Request, - buildPayload: (request: Request) => Promise -): void { - if (catalogInFlight.has(cacheKey)) return; // a refresh for this key is already running - - const refreshPromise: Promise = new Promise((resolve, reject) => { - setTimeout(() => { - runBuilder(buildPayload, request) - .then((payload) => resolve(storePayload(cacheKey, payload))) - .catch((err) => { - console.error( - `[catalog] Background stale-while-revalidate refresh failed for key "${cacheKey}":`, - err - ); - reject(err); - }); - }, 0); }); - - // Nobody on the stale path awaits this, so pre-handle the rejection; a cold-path - // caller that joins it via catalogInFlight attaches its own handler and still - // observes the failure. - refreshPromise.catch(() => {}); - - catalogInFlight.set(cacheKey, refreshPromise); - refreshPromise - .catch(() => {}) - .finally(() => { - if (catalogInFlight.get(cacheKey) === refreshPromise) catalogInFlight.delete(cacheKey); - }); } function runBuilder( @@ -183,23 +163,104 @@ function runBuilder( request: Request ): Promise { _catalogBuilderRuns++; - return buildPayload(request); + try { + return Promise.resolve(buildPayload(request)); + } catch (error) { + return Promise.reject(error); + } +} + +function cleanInFlight(cacheKey: string, inFlight: CatalogInFlight): void { + if (catalogInFlight.get(cacheKey) === inFlight) { + catalogInFlight.delete(cacheKey); + } +} + +function startSynchronousBuild( + cacheKey: string, + request: Request, + buildPayload: (request: Request) => Promise +): CatalogInFlight { + const generation = catalogGeneration; + let inFlight!: CatalogInFlight; + const promise = runBuilder(buildPayload, request).then((payload) => { + storeSuccessfulPayload(cacheKey, payload, inFlight); + return payload; + }); + inFlight = { generation, promise }; + catalogInFlight.set(cacheKey, inFlight); + inFlight.promise.then( + () => cleanInFlight(cacheKey, inFlight), + () => cleanInFlight(cacheKey, inFlight) + ); + return inFlight; +} + +function scheduleBackgroundRefresh( + cacheKey: string, + request: Request, + buildPayload: (request: Request) => Promise, + schedule: CatalogRefreshScheduler +): void { + if (catalogInFlight.has(cacheKey)) return; + + let resolveRefresh!: (payload: CatalogPayload) => void; + let rejectRefresh!: (error: unknown) => void; + const inFlight: CatalogInFlight = { + generation: catalogGeneration, + promise: new Promise((resolve, reject) => { + resolveRefresh = resolve; + rejectRefresh = reject; + }), + }; + + // Reserve the key before handing the task to the scheduler. Multiple stale reads in + // the same request turn therefore cannot enqueue duplicate refreshes. + catalogInFlight.set(cacheKey, inFlight); + void inFlight.promise.catch(() => {}); // background failures are always handled + + const task: CatalogRefreshTask = async () => { + synchronizeCatalogGeneration(); + if (inFlight.generation !== catalogGeneration || catalogInFlight.get(cacheKey) !== inFlight) { + // Hard invalidation or a deterministic test reset detached this scheduled task + // before it started. Resolve its private bookkeeping promise without rebuilding. + resolveRefresh({ body: "", headers: {}, status: 204, cacheTTL: 0 }); + return; + } + + try { + const payload = await runBuilder(buildPayload, request); + storeSuccessfulPayload(cacheKey, payload, inFlight); + resolveRefresh(payload); + } catch (error) { + console.error("[catalog] Background stale-while-revalidate refresh failed:", error); + rejectRefresh(error); + } finally { + cleanInFlight(cacheKey, inFlight); + } + }; + + try { + schedule(task); + } catch (error) { + cleanInFlight(cacheKey, inFlight); + rejectRefresh(error); + console.error("[catalog] Failed to schedule background refresh:", error); + } } /** - * Resolve the cached catalog response for `request`, building it through - * `buildPayload` when there is nothing fresh to serve. - * - * Returns `null` when the caller must build and handle errors itself — i.e. the - * in-flight build rejected — so the error-response shape stays in the caller. + * Resolve the cached catalog response for `request`, building it through the shared + * `buildPayload` primitive when there is no current snapshot. */ export async function resolveCachedCatalogResponse( request: Request, headerSources: { corsHeaders: Record; diagnosticHeaders: Record }, - buildPayload: (request: Request) => Promise + buildPayload: (request: Request) => Promise, + policy: CatalogCachePolicy = {} ): Promise { const { corsHeaders, diagnosticHeaders } = headerSources; - dropCatalogCacheIfStateChanged(); + synchronizeCatalogGeneration(); const cacheKey = buildCatalogCacheKey(request); const now = Date.now(); @@ -212,33 +273,32 @@ export async function resolveCachedCatalogResponse( }); } - // Stale-while-revalidate: an expired entry is still served immediately as long as - // (a) it was a successful build — a cached error replayed as "stale" would mask an - // intermittent failure behind a fake success forever — and (b) it is within the - // staleness window, so a refresh that keeps failing eventually falls through to the - // cold-path wait instead of pinning ancient data. + const staleWhileRevalidateMs = + policy.getStaleWhileRevalidateMs?.() ?? getCatalogStaleWhileRevalidateMs(); if ( cached && - cached.status === 200 && - now - cached.expiresAt <= CATALOG_STALE_WHILE_REVALIDATE_MS + cached.status >= 200 && + cached.status < 300 && + now - cached.expiresAt <= staleWhileRevalidateMs ) { - scheduleBackgroundRefresh(cacheKey, request, buildPayload); + scheduleBackgroundRefresh( + cacheKey, + request, + buildPayload, + policy.scheduleBackgroundRefresh ?? defaultBackgroundRefreshScheduler + ); return new Response(cached.body, { status: cached.status, headers: mergeCatalogHeaders(corsHeaders, cached.headers, diagnosticHeaders), }); } - let inflight = catalogInFlight.get(cacheKey); - if (!inflight) { - inflight = runBuilder(buildPayload, request).then((payload) => storePayload(cacheKey, payload)); - catalogInFlight.set(cacheKey, inflight); - inflight.finally(() => { - if (catalogInFlight.get(cacheKey) === inflight) catalogInFlight.delete(cacheKey); - }); + let inFlight = catalogInFlight.get(cacheKey); + if (!inFlight) { + inFlight = startSynchronousBuild(cacheKey, request, buildPayload); } - const payload = await inflight; + const payload = await inFlight.promise; return new Response(payload.body, { status: payload.status, headers: mergeCatalogHeaders(corsHeaders, payload.headers, diagnosticHeaders), @@ -246,14 +306,26 @@ export async function resolveCachedCatalogResponse( } // ── Test hooks ─────────────────────────────────────────────────────────────── -// Not part of the public API; do not read from app code. +// Not part of the public application API. -/** Resets the builder counter and every cached/in-flight entry. */ +/** Deterministically resets counters, policy, snapshots, generations, and old work. */ export function __resetCatalogBuilderRunsForTest(): void { _catalogBuilderRuns = 0; + catalogGeneration++; catalogCache.clear(); catalogInFlight.clear(); lastSeenCatalogCacheVersion = getModelCatalogCacheVersion(); + staleWhileRevalidateMsAccessor = () => CATALOG_STALE_WHILE_REVALIDATE_MS; +} + +/** Injects the SWR policy accessor without environment-dependent behavior. */ +export function __setCatalogStaleWhileRevalidateAccessorForTest(accessor: () => number): void { + staleWhileRevalidateMsAccessor = accessor; +} + +/** Backward-compatible scalar policy hook retained for focused tests. */ +export function __setCatalogStaleWhileRevalidateMsForTest(ms: number): void { + staleWhileRevalidateMsAccessor = () => ms; } /** Counts full builder executions — proves concurrent requests share one run (#6408). */ @@ -261,11 +333,7 @@ export function __getCatalogBuilderRunsForTest(): number { return _catalogBuilderRuns; } -/** - * Marks every cached entry as expired `msAgo` milliseconds ago instead of sleeping - * out the real TTL. Pass more than CATALOG_STALE_WHILE_REVALIDATE_MS to simulate an - * entry that has aged past the stale-serving window. - */ +/** Marks every successful snapshot expired without sleeping out the real TTL. */ export function __expireCatalogCacheForTest(msAgo = 1): void { const expiresAt = Date.now() - msAgo; for (const [key, entry] of catalogCache.entries()) { @@ -273,33 +341,22 @@ export function __expireCatalogCacheForTest(msAgo = 1): void { } } -/** - * Seeds the entry a given request would read, for status/staleness combinations the - * intentionally exception-resistant builder cannot be made to produce (e.g. a cached - * non-200). Takes the Request so the cache-key format stays private to this module. - */ +/** Seeds a request-keyed snapshot for status/staleness compatibility tests. */ export function __setCatalogCacheEntryForTest(request: Request, entry: CachedCatalog): void { catalogCache.set(buildCatalogCacheKey(request), entry); } -/** Awaits any background refresh in flight, instead of guessing at a real-time sleep. */ +/** Awaits any currently running or scheduled refresh without real-time sleeps. */ export async function __flushCatalogBackgroundRefreshForTest(): Promise { - await Promise.all([...catalogInFlight.values()].map((p) => p.catch(() => {}))); + await Promise.all([...catalogInFlight.values()].map(({ promise }) => promise.catch(() => {}))); } -/** - * Injects a synthetic in-flight rejection so the caller's catch branch (sanitized - * error body) can be exercised deterministically — the builder core try/catches every - * registry and DB read individually, so it is not a practical error-injection point. - * - * Deliberately does not self-clean the way production entries do: this promise is - * already rejected at creation, so a cleanup callback would delete the map entry - * within a microtask or two — before the caller's several-await auth check finishes — - * silently swapping in a fresh cold build instead of the intended failure. The next - * __resetCatalogBuilderRunsForTest() clears it. - */ +/** Injects a handled in-flight rejection for the catalog error-shape regression test. */ export function __forceCatalogInFlightRejectionForTest(request: Request, error: unknown): void { - const rejected: Promise = Promise.reject(error); - rejected.catch(() => {}); // mark as handled — avoids an unhandledRejection warning - catalogInFlight.set(buildCatalogCacheKey(request), rejected); + const promise: Promise = Promise.reject(error); + void promise.catch(() => {}); + catalogInFlight.set(buildCatalogCacheKey(request), { + generation: catalogGeneration, + promise, + }); } diff --git a/src/app/api/v1/models/route.ts b/src/app/api/v1/models/route.ts index f884388ae8..da37ca65e8 100644 --- a/src/app/api/v1/models/route.ts +++ b/src/app/api/v1/models/route.ts @@ -1,3 +1,5 @@ +import { after } from "next/server"; + import { getUnifiedModelsResponse } from "./catalog"; /** @@ -31,5 +33,11 @@ export async function HEAD() { * GET /v1/models - OpenAI compatible models list */ export async function GET(request: Request) { - return getUnifiedModelsResponse(request); + return getUnifiedModelsResponse( + request, + {}, + { + scheduleBackgroundRefresh: (task) => after(task), + } + ); } diff --git a/src/lib/db/models.ts b/src/lib/db/models.ts index 168a71d47a..2c21f26619 100644 --- a/src/lib/db/models.ts +++ b/src/lib/db/models.ts @@ -8,6 +8,10 @@ import { getDbInstance } from "./core"; import { backupDbFile } from "./backup"; import { getProviderConnectionsCount } from "./providers"; import { type JsonRecord, asRecord, toNonEmptyString, getKeyValue } from "./models/shared"; +import { + finishSyncedAvailableModelsWrite, + persistCanonicalSyncedAvailableModels, +} from "./models/syncedAvailableModelPersistence"; import { readCompatList, writeCompatList, @@ -561,7 +565,6 @@ export async function replaceSyncedAvailableModelsForConnection( connectionId: string, models: SyncedAvailableModelInput[] ): Promise { - const db = getDbInstance(); const key = `${providerId}:${connectionId}`; // #3199: drop ids the operator DELETED (trash) so a re-fetch does not re-import // a model that was explicitly removed. @@ -573,16 +576,7 @@ export async function replaceSyncedAvailableModelsForConnection( const normalizedModels = normalizeSyncedAvailableModels(models).filter( (m) => !getModelIsDeleted(providerId, m.id) ); - if (normalizedModels.length === 0) { - db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run( - key - ); - } else { - db.prepare( - "INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES ('syncedAvailableModels', ?, ?)" - ).run(key, JSON.stringify(normalizedModels)); - } - backupDbFile("pre-write"); + persistCanonicalSyncedAvailableModels(key, normalizedModels, normalizeSyncedAvailableModels); // Return the full unioned list for the provider return getSyncedAvailableModels(providerId); } @@ -632,11 +626,10 @@ export async function removeSyncedAvailableModel( } } } - - if (removedAny) backupDbFile("pre-write"); }); removeModel(); + if (removedAny) finishSyncedAvailableModelsWrite(); return removedAny; } @@ -650,10 +643,10 @@ export async function deleteSyncedAvailableModelsForConnection( ): Promise { const db = getDbInstance(); const key = `${providerId}:${connectionId}`; - db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run( - key - ); - backupDbFile("pre-write"); + const result = db + .prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?") + .run(key); + if (result.changes > 0) finishSyncedAvailableModelsWrite(); return getSyncedAvailableModels(providerId); } @@ -692,8 +685,9 @@ export async function deleteSyncedAvailableModelsForProvider(providerId: string) "DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND substr(key, 1, ?) = ?" ) .run(keyPrefix.length, keyPrefix); - backupDbFile("pre-write"); - return Number(result.changes || 0); + const changes = Number(result.changes || 0); + if (changes > 0) finishSyncedAvailableModelsWrite(); + return changes; } /** @@ -716,8 +710,9 @@ export async function pruneStaleSyncedAvailableModelsForProvider( `DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key LIKE ? AND key NOT IN (${placeholders})` ) .run(`${keyPrefix}%`, ...allowedKeys); - backupDbFile("pre-write"); - return Number(result.changes || 0); + const changes = Number(result.changes || 0); + if (changes > 0) finishSyncedAvailableModelsWrite(); + return changes; } /** diff --git a/src/lib/db/models/syncedAvailableModelPersistence.ts b/src/lib/db/models/syncedAvailableModelPersistence.ts new file mode 100644 index 0000000000..7e8d13a142 --- /dev/null +++ b/src/lib/db/models/syncedAvailableModelPersistence.ts @@ -0,0 +1,52 @@ +/** Canonical comparison and write helpers for connection-scoped synced model catalogs. */ + +import { backupDbFile } from "../backup"; +import { getDbInstance } from "../core"; +import { invalidateModelCatalogCache } from "../readCache"; +import { getKeyValue } from "./shared"; + +type ModelNormalizer = (models: unknown) => T[]; + +export function finishSyncedAvailableModelsWrite(): void { + backupDbFile("pre-write"); + invalidateModelCatalogCache(); +} + +export function persistCanonicalSyncedAvailableModels( + key: string, + normalizedModels: T[], + normalizeModels: ModelNormalizer +): boolean { + const db = getDbInstance(); + const existingRow = db + .prepare("SELECT value FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?") + .get(key); + const existingValue = getKeyValue(existingRow).value; + let existingModels: T[] | null = null; + if (existingValue !== null) { + try { + existingModels = normalizeModels(JSON.parse(existingValue)); + } catch { + existingModels = null; + } + } + + const unchanged = + (normalizedModels.length > 0 && + existingModels !== null && + JSON.stringify(existingModels) === JSON.stringify(normalizedModels)) || + (normalizedModels.length === 0 && existingValue === null); + if (unchanged) return false; + + if (normalizedModels.length === 0) { + db.prepare("DELETE FROM key_value WHERE namespace = 'syncedAvailableModels' AND key = ?").run( + key + ); + } else { + db.prepare( + "INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES ('syncedAvailableModels', ?, ?)" + ).run(key, JSON.stringify(normalizedModels)); + } + finishSyncedAvailableModelsWrite(); + return true; +} diff --git a/src/lib/db/readCache.ts b/src/lib/db/readCache.ts index b8f520fe19..e6072be174 100644 --- a/src/lib/db/readCache.ts +++ b/src/lib/db/readCache.ts @@ -233,26 +233,29 @@ export function getCombosCacheVersion(): number { // ──────────────── Model Catalog Cache Invalidation Signal ──────────────── // -// #6408 added a request-shape-keyed (prefix/isCodex/apiKey) TTL cache around the -// unified /v1/models builder (src/app/api/v1/models/catalog.ts) to coalesce -// concurrent/bursty GETs. That cache key does not vary with the underlying DB -// state the builder reads (connections, settings, combos), so a write followed by -// a read within the ~1.5s TTL replayed the pre-write response. Same import-cycle +// #6408 added a request-shape-keyed (prefix/isCodex/apiKey/configuredOnly) TTL +// cache around the unified /v1/models builder (src/app/api/v1/models/catalog.ts) +// to coalesce concurrent/bursty GETs. That cache key does not vary with the +// underlying DB state the builder reads (connections, settings, combos), so +// writes need an explicit invalidation signal. Same import-cycle // constraint as combosCacheVersion above (a db module must not import the route -// module) — catalog.ts instead compares this version on every access and drops its -// whole cache the moment it moves, so any write that calls invalidateDbCache() makes -// the next read miss immediately instead of waiting out the TTL. +// module): catalogCache.ts compares this version on every access and builder +// completion, then hard-invalidates snapshots and old-generation work when it moves. let modelCatalogCacheVersion = 0; /** - * Current model-catalog-cache version. `getUnifiedModelsResponse()` folds this - * into its response cache key; a change means settings/connections/combos were - * written since the cache was populated and the cached body is stale. + * Current model-catalog-cache version. A change means catalog-backed state was + * written and the next read must synchronously build the new generation. */ export function getModelCatalogCacheVersion(): number { return modelCatalogCacheVersion; } +/** Invalidate only the unified model catalog response cache. */ +export function invalidateModelCatalogCache(): void { + modelCatalogCacheVersion++; +} + /** * Invalidate caches (call after writes to any of: settings, pricing, * connections, combos, nodes). diff --git a/tests/integration/v1-models-swr-response-flush-8728.test.ts b/tests/integration/v1-models-swr-response-flush-8728.test.ts new file mode 100644 index 0000000000..60aa72c894 --- /dev/null +++ b/tests/integration/v1-models-swr-response-flush-8728.test.ts @@ -0,0 +1,183 @@ +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import fs from "node:fs"; +import http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; + +const CACHE_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-swr-cache-8728-")); +process.env.DATA_DIR = CACHE_DATA_DIR; + +const catalogCache = await import("../../src/app/api/v1/models/catalogCache.ts"); +const REPO_ROOT = fileURLToPath(new URL("../..", import.meta.url)); +const BLOCK_MS = 300; + +function listen(server: http.Server, socketPath: string): Promise { + return new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(socketPath, resolve); + }); +} + +function close(server: http.Server): Promise { + return new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); +} + +function fetchFromExternalClient( + socketPath: string +): Promise<{ body: string; receivedAt: number }> { + const script = [ + 'import http from "node:http";', + "const chunks = [];", + "const request = http.request({ socketPath: process.argv[1], path: '/v1/models' }, (response) => {", + " response.on('data', (chunk) => chunks.push(chunk));", + " response.on('end', () => {", + " const body = Buffer.concat(chunks).toString('utf8');", + " process.stdout.write(JSON.stringify({ body, receivedAt: Date.now() }));", + " });", + "});", + "request.on('error', (error) => { throw error; });", + "request.end();", + ].join("\n"); + const child = spawn(process.execPath, ["--input-type=module", "-e", script, socketPath], { + stdio: ["ignore", "pipe", "pipe"], + }); + + return new Promise((resolve, reject) => { + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (chunk) => { + stdout += String(chunk); + }); + child.stderr.on("data", (chunk) => { + stderr += String(chunk); + }); + child.once("error", reject); + child.once("exit", (code) => { + if (code !== 0) { + reject(new Error(`external fetch exited ${code}: ${stderr}`)); + return; + } + resolve(JSON.parse(stdout)); + }); + }); +} + +function productionShapedSynchronousRefresh() { + const models = Array.from({ length: 4_000 }, (_, index) => ({ + id: `provider/model-${index}`, + object: "model", + owned_by: "provider", + display_name: `Model ${index}`, + })); + const startedAt = Date.now(); + do { + JSON.stringify({ object: "list", data: models }); + } while (Date.now() - startedAt < BLOCK_MS); +} + +test.after(() => { + fs.rmSync(CACHE_DATA_DIR, { recursive: true, force: true }); +}); + +test("the /v1/models route wires Next after() as its response-flush-safe scheduler", () => { + const routeSource = fs.readFileSync( + path.join(REPO_ROOT, "src/app/api/v1/models/route.ts"), + "utf8" + ); + const cacheSource = fs.readFileSync( + path.join(REPO_ROOT, "src/app/api/v1/models/catalogCache.ts"), + "utf8" + ); + assert.match(routeSource, /import\s+\{\s*after\s*\}\s+from\s+["']next\/server["']/); + assert.match(routeSource, /scheduleBackgroundRefresh:\s*\(task\)\s*=>\s*after\(task\)/); + assert.match(cacheSource, /import\s+\{\s*after\s*\}\s+from\s+["']next\/server["']/); + assert.match(cacheSource, /function defaultBackgroundRefreshScheduler[\s\S]*?after\(task\)/); +}); + +test("an external client receives the stale body before synchronous refresh finishes blocking", async (t) => { + catalogCache.__resetCatalogBuilderRunsForTest(); + const socketDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-swr-http-8728-")); + const socketPath = path.join(socketDir, "catalog.sock"); + + let buildCount = 0; + let responseFinishedAt = 0; + let refreshStartedAt = 0; + let refreshFinishedAt = 0; + let scheduledCount = 0; + + const server = http.createServer(async (incoming, outgoing) => { + const url = `http://127.0.0.1${incoming.url || "/"}`; + const response = await catalogCache.resolveCachedCatalogResponse( + new Request(url), + { corsHeaders: {}, diagnosticHeaders: {} }, + async () => { + buildCount++; + if (buildCount > 1) { + refreshStartedAt = Date.now(); + productionShapedSynchronousRefresh(); + refreshFinishedAt = Date.now(); + } + return { + body: buildCount > 1 ? "new" : "old", + headers: { "content-type": "text/plain" }, + status: 200, + cacheTTL: 60_000, + }; + }, + { + getStaleWhileRevalidateMs: () => Number.POSITIVE_INFINITY, + scheduleBackgroundRefresh: (task) => { + scheduledCount++; + outgoing.once("finish", () => { + responseFinishedAt = Date.now(); + setImmediate(() => { + void task(); + }); + }); + }, + } + ); + + outgoing.writeHead(response.status, Object.fromEntries(response.headers.entries())); + outgoing.end(await response.text()); + }); + + try { + await listen(server, socketPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EPERM") { + t.skip("sandbox does not permit opening HTTP listener sockets"); + fs.rmSync(socketDir, { recursive: true, force: true }); + return; + } + throw error; + } + try { + assert.equal((await fetchFromExternalClient(socketPath)).body, "old"); + catalogCache.__expireCatalogCacheForTest(); + + const stale = await fetchFromExternalClient(socketPath); + await catalogCache.__flushCatalogBackgroundRefreshForTest(); + + assert.equal(stale.body, "old"); + assert.equal(scheduledCount, 1); + assert.ok(refreshStartedAt >= responseFinishedAt, "refresh must start after response finish"); + assert.ok( + stale.receivedAt < refreshFinishedAt, + `external client received stale body at ${stale.receivedAt}, after refresh finished at ${refreshFinishedAt}` + ); + assert.ok( + refreshFinishedAt - refreshStartedAt >= BLOCK_MS, + "refresh did not exercise the synchronous blocking window" + ); + } finally { + await close(server); + fs.rmSync(socketDir, { recursive: true, force: true }); + catalogCache.__resetCatalogBuilderRunsForTest(); + } +}); diff --git a/tests/unit/db-synced-model-catalog-invalidation-8728.test.ts b/tests/unit/db-synced-model-catalog-invalidation-8728.test.ts new file mode 100644 index 0000000000..f013d530ff --- /dev/null +++ b/tests/unit/db-synced-model-catalog-invalidation-8728.test.ts @@ -0,0 +1,166 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-db-model-cache-8728-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.DISABLE_SQLITE_AUTO_BACKUP = "true"; + +const core = await import("../../src/lib/db/core.ts"); +const models = await import("../../src/lib/db/models.ts"); +const readCache = await import("../../src/lib/db/readCache.ts"); + +function resetStorage() { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +function catalogVersion() { + return readCache.getModelCatalogCacheVersion(); +} + +function totalChanges() { + return Number( + ( + core.getDbInstance().prepare("SELECT total_changes() AS count").get() as { + count: number; + } + ).count + ); +} + +function seedSynced(providerId: string, connectionId: string, value: unknown) { + core + .getDbInstance() + .prepare("INSERT INTO key_value (namespace, key, value) VALUES ('syncedAvailableModels', ?, ?)") + .run(`${providerId}:${connectionId}`, JSON.stringify(value)); +} + +test.beforeEach(() => { + resetStorage(); +}); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("replace invalidates only for canonical persisted changes", async () => { + const initialVersion = catalogVersion(); + + await models.replaceSyncedAvailableModelsForConnection("openai", "a", [ + { + id: "gpt-b", + name: " GPT B ", + source: "ignored", + supportedEndpoints: ["responses", "chat", "chat"], + }, + { id: "gpt-a", name: "GPT A" }, + ]); + assert.equal(catalogVersion(), initialVersion + 1); + + const beforeIdenticalChanges = totalChanges(); + const beforeIdenticalVersion = catalogVersion(); + await models.replaceSyncedAvailableModelsForConnection("openai", "a", [ + { + id: "gpt-b", + name: "GPT B", + source: "imported", + supportedEndpoints: ["chat", "responses"], + }, + { id: "gpt-a", name: "GPT A", source: "imported" }, + ]); + assert.equal(totalChanges(), beforeIdenticalChanges, "canonical equality must skip the DB write"); + assert.equal(catalogVersion(), beforeIdenticalVersion, "canonical equality must not invalidate"); + + await models.replaceSyncedAvailableModelsForConnection("openai", "a", [ + { id: "gpt-a", name: "GPT A" }, + { id: "gpt-b", name: "GPT B", supportedEndpoints: ["chat", "responses"] }, + ]); + assert.equal(catalogVersion(), beforeIdenticalVersion + 1, "ordering changes are persisted"); + + await models.replaceSyncedAvailableModelsForConnection("openai", "a", [ + { id: "gpt-a", name: "GPT A renamed" }, + { id: "gpt-b", name: "GPT B", supportedEndpoints: ["chat", "responses"] }, + ]); + assert.equal(catalogVersion(), beforeIdenticalVersion + 2, "content changes are persisted"); +}); + +test("replace handles empty and deleted-model normalization without phantom writes", async () => { + const startVersion = catalogVersion(); + const startChanges = totalChanges(); + + await models.replaceSyncedAvailableModelsForConnection("openai", "absent", []); + assert.equal(totalChanges(), startChanges, "absent-to-empty replacement must be a no-op"); + assert.equal(catalogVersion(), startVersion); + + models.mergeModelCompatOverride("openai", "trashed", { + isDeleted: true, + isHidden: true, + }); + + const beforeDeletedAbsent = totalChanges(); + const beforeDeletedVersion = catalogVersion(); + await models.replaceSyncedAvailableModelsForConnection("openai", "absent", [ + { id: "trashed", name: "Trashed" }, + ]); + assert.equal(totalChanges(), beforeDeletedAbsent, "filtered deleted models must stay absent"); + assert.equal(catalogVersion(), beforeDeletedVersion); + + seedSynced("openai", "present", [{ id: "existing", name: "Existing" }]); + const beforeDeleteVersion = catalogVersion(); + await models.replaceSyncedAvailableModelsForConnection("openai", "present", []); + assert.equal(catalogVersion(), beforeDeleteVersion + 1, "present-to-empty must invalidate"); + assert.deepEqual(await models.getSyncedAvailableModelsForConnection("openai", "present"), []); +}); + +test("remove and connection/provider deletes invalidate only when rows actually change", async () => { + let version = catalogVersion(); + assert.equal(await models.removeSyncedAvailableModel("openai", "missing"), false); + assert.equal(catalogVersion(), version); + + seedSynced("openai", "a", [ + { id: "gpt-a", name: "GPT A" }, + { id: "gpt-b", name: "GPT B" }, + ]); + assert.equal(await models.removeSyncedAvailableModel("openai", "gpt-a"), true); + assert.equal(catalogVersion(), ++version); + assert.equal(await models.removeSyncedAvailableModel("openai", "gpt-a"), false); + assert.equal(catalogVersion(), version); + + await models.deleteSyncedAvailableModelsForConnection("openai", "missing"); + assert.equal(catalogVersion(), version); + await models.deleteSyncedAvailableModelsForConnection("openai", "a"); + assert.equal(catalogVersion(), ++version); + + assert.equal(await models.deleteSyncedAvailableModelsForProvider("openai"), 0); + assert.equal(catalogVersion(), version); + seedSynced("openai", "b", [{ id: "gpt-b", name: "GPT B" }]); + seedSynced("openai", "c", [{ id: "gpt-c", name: "GPT C" }]); + assert.equal(await models.deleteSyncedAvailableModelsForProvider("openai"), 2); + assert.equal(catalogVersion(), ++version); +}); + +test("prune uses affected rows and the delegated provider delete invalidates exactly once", async () => { + seedSynced("openai", "keep", [{ id: "gpt-a", name: "GPT A" }]); + seedSynced("openai", "stale", [{ id: "gpt-b", name: "GPT B" }]); + let version = catalogVersion(); + + assert.equal( + await models.pruneStaleSyncedAvailableModelsForProvider("openai", ["keep", "stale"]), + 0 + ); + assert.equal(catalogVersion(), version); + + assert.equal(await models.pruneStaleSyncedAvailableModelsForProvider("openai", ["keep"]), 1); + assert.equal(catalogVersion(), ++version); + + assert.equal(await models.pruneStaleSyncedAvailableModelsForProvider("openai", []), 1); + assert.equal(catalogVersion(), ++version, "delegated provider delete must invalidate once"); + + assert.equal(await models.pruneStaleSyncedAvailableModelsForProvider("openai", []), 0); + assert.equal(catalogVersion(), version); +}); diff --git a/tests/unit/model-catalog-cache-swr-8728.test.ts b/tests/unit/model-catalog-cache-swr-8728.test.ts new file mode 100644 index 0000000000..ffef72a217 --- /dev/null +++ b/tests/unit/model-catalog-cache-swr-8728.test.ts @@ -0,0 +1,203 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-catalog-cache-8728-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const readCache = await import("../../src/lib/db/readCache.ts"); +const catalogCache = await import("../../src/app/api/v1/models/catalogCache.ts"); + +type RefreshTask = () => Promise; + +function request() { + return new Request("http://localhost/v1/models"); +} + +function payload(body: string, status = 200): catalogCache.CatalogPayload { + return { + body, + headers: { "content-type": "application/json" }, + status, + cacheTTL: 60_000, + }; +} + +function createPolicyQueue() { + const tasks: RefreshTask[] = []; + return { + policy: { + getStaleWhileRevalidateMs: () => Number.POSITIVE_INFINITY, + scheduleBackgroundRefresh: (task: RefreshTask) => { + tasks.push(task); + }, + }, + tasks, + }; +} + +async function resolve( + build: (request: Request) => Promise, + policy = createPolicyQueue().policy +) { + return catalogCache.resolveCachedCatalogResponse( + request(), + { corsHeaders: {}, diagnosticHeaders: {} }, + build, + policy + ); +} + +test.beforeEach(() => { + catalogCache.__resetCatalogBuilderRunsForTest(); +}); + +test.after(() => { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("production SWR policy is unbounded and reset restores the default accessor", () => { + assert.equal(catalogCache.CATALOG_STALE_WHILE_REVALIDATE_MS, Number.POSITIVE_INFINITY); + assert.equal(catalogCache.getCatalogStaleWhileRevalidateMs(), Number.POSITIVE_INFINITY); + + catalogCache.__setCatalogStaleWhileRevalidateAccessorForTest(() => 0); + assert.equal(catalogCache.getCatalogStaleWhileRevalidateMs(), 0); + + catalogCache.__resetCatalogBuilderRunsForTest(); + assert.equal(catalogCache.getCatalogStaleWhileRevalidateMs(), Number.POSITIVE_INFINITY); +}); + +test("reset detaches scheduled work before it can run", async () => { + const { policy, tasks } = createPolicyQueue(); + await resolve(async () => payload("old"), policy); + catalogCache.__expireCatalogCacheForTest(); + await resolve(async () => payload("detached"), policy); + assert.equal(tasks.length, 1); + + catalogCache.__resetCatalogBuilderRunsForTest(); + await tasks[0](); + + assert.equal(catalogCache.__getCatalogBuilderRunsForTest(), 0); +}); + +test("ordinary TTL expiry serves the last success indefinitely and schedules one refresh per key", async () => { + const { policy, tasks } = createPolicyQueue(); + const initial = await resolve(async () => payload("old"), policy); + assert.equal(await initial.text(), "old"); + catalogCache.__expireCatalogCacheForTest(7 * 24 * 60 * 60 * 1000); + + const staleResponses = await Promise.all( + Array.from({ length: 5 }, () => resolve(async () => payload("new"), policy)) + ); + + assert.deepEqual( + await Promise.all(staleResponses.map((response) => response.text())), + Array(5).fill("old") + ); + assert.equal(tasks.length, 1, "concurrent stale reads must schedule exactly one refresh"); + assert.equal(catalogCache.__getCatalogBuilderRunsForTest(), 1); + + await tasks[0](); + + const refreshed = await resolve(async () => payload("unexpected"), policy); + assert.equal(await refreshed.text(), "new"); + assert.equal(catalogCache.__getCatalogBuilderRunsForTest(), 2); +}); + +test("unsuccessful cold payloads are returned but never cached", async () => { + const first = await resolve(async () => payload("temporary failure", 503)); + assert.equal(first.status, 503); + assert.equal(await first.text(), "temporary failure"); + + const second = await resolve(async () => payload("recovered")); + assert.equal(second.status, 200); + assert.equal(await second.text(), "recovered"); + assert.equal(catalogCache.__getCatalogBuilderRunsForTest(), 2); +}); + +test("failed background refresh retains the prior successful snapshot and permits retry", async (t) => { + t.mock.method(console, "error", () => {}); + const { policy, tasks } = createPolicyQueue(); + assert.equal(await (await resolve(async () => payload("old"), policy)).text(), "old"); + catalogCache.__expireCatalogCacheForTest(); + + assert.equal( + await ( + await resolve(async () => { + throw new Error("temporary failure"); + }, policy) + ).text(), + "old" + ); + await tasks.shift()!(); + + assert.equal( + await (await resolve(async () => payload("temporary failure", 503), policy)).text(), + "old" + ); + assert.equal(tasks.length, 1, "a failed refresh must release single-flight state for retry"); + await tasks.shift()!(); + + assert.equal(await (await resolve(async () => payload("new"), policy)).text(), "old"); + assert.equal(tasks.length, 1, "an unsuccessful payload must also permit another refresh"); + await tasks.shift()!(); + + assert.equal(await (await resolve(async () => payload("unused"), policy)).text(), "new"); +}); + +test("hard invalidation drops snapshots, detaches old work, and guards old-generation writeback", async () => { + let resolveOld!: (value: catalogCache.CatalogPayload) => void; + const oldPayload = new Promise((resolvePromise) => { + resolveOld = resolvePromise; + }); + let currentBuildStarted = false; + let resolveCurrent!: (value: catalogCache.CatalogPayload) => void; + const currentPayload = new Promise((resolvePromise) => { + resolveCurrent = resolvePromise; + }); + + const oldRequest = resolve(async () => oldPayload); + await Promise.resolve(); + + readCache.invalidateModelCatalogCache(); + const currentRequest = resolve(async () => { + currentBuildStarted = true; + return currentPayload; + }); + await Promise.resolve(); + + assert.equal(currentBuildStarted, true, "the first post-write read must start a current build"); + + resolveCurrent(payload("current")); + assert.equal(await (await currentRequest).text(), "current"); + + resolveOld(payload("old")); + assert.equal(await (await oldRequest).text(), "old"); + + const cached = await resolve(async () => payload("unexpected")); + assert.equal(await cached.text(), "current", "old completion must not overwrite current cache"); + assert.equal(catalogCache.__getCatalogBuilderRunsForTest(), 2); +}); + +test("hard invalidation clears a completed snapshot and makes the next read block", async () => { + assert.equal(await (await resolve(async () => payload("old"))).text(), "old"); + readCache.invalidateModelCatalogCache(); + + let resolveCurrent!: (value: catalogCache.CatalogPayload) => void; + const currentPayload = new Promise((resolvePromise) => { + resolveCurrent = resolvePromise; + }); + let settled = false; + const next = resolve(async () => currentPayload).then((response) => { + settled = true; + return response; + }); + + await Promise.resolve(); + assert.equal(settled, false, "post-write reads may block and must not serve the old snapshot"); + + resolveCurrent(payload("current")); + assert.equal(await (await next).text(), "current"); +}); diff --git a/tests/unit/quota-exclusive-catalog-short-circuit.test.ts b/tests/unit/quota-exclusive-catalog-short-circuit.test.ts index c3710a1cff..f6b786d643 100644 --- a/tests/unit/quota-exclusive-catalog-short-circuit.test.ts +++ b/tests/unit/quota-exclusive-catalog-short-circuit.test.ts @@ -134,9 +134,8 @@ await test("chave quota-exclusive não constrói o catálogo completo", async (t process.env.EXPOSE_CC_DISCOVERY_ALIASES = "1"; // A chave do cache é `prefix|isCodex|apiKey|configuredOnly` — um query param // qualquer NÃO a invalida, então a resposta do subteste anterior seria servida. - v1ModelsCatalog.__expireCatalogCacheForTest( - v1ModelsCatalog.CATALOG_STALE_WHILE_REVALIDATE_MS + 1000 - ); + v1ModelsCatalog.__setCatalogStaleWhileRevalidateMsForTest(0); + v1ModelsCatalog.__expireCatalogCacheForTest(1); try { const res = await v1ModelsCatalog.getUnifiedModelsResponse( new Request("http://localhost/api/v1/models", { @@ -152,6 +151,9 @@ await test("chave quota-exclusive não constrói o catálogo completo", async (t `ids=${JSON.stringify(body.data.map((m) => m.id).slice(0, 6))}` ); } finally { + v1ModelsCatalog.__setCatalogStaleWhileRevalidateMsForTest( + v1ModelsCatalog.CATALOG_STALE_WHILE_REVALIDATE_MS + ); if (prev === undefined) delete process.env.EXPOSE_CC_DISCOVERY_ALIASES; else process.env.EXPOSE_CC_DISCOVERY_ALIASES = prev; } diff --git a/tests/unit/v1-models-discovery-conformance.test.ts b/tests/unit/v1-models-discovery-conformance.test.ts index e1ab778003..9941d18ca5 100644 --- a/tests/unit/v1-models-discovery-conformance.test.ts +++ b/tests/unit/v1-models-discovery-conformance.test.ts @@ -108,27 +108,29 @@ test("3. stale-first: an expired 200 entry within the staleness window is served ); }); -test("4. beyond the staleness window, the response waits for a fresh build again", async () => { +test("4. an ordinary TTL expiry remains stale-first regardless of snapshot age", async () => { const makeRequest = () => new Request("http://localhost/v1/models"); const res1 = await v1ModelsCatalog.getUnifiedModelsResponse(makeRequest()); assert.equal(res1.status, 200); + const body1 = await res1.text(); const runsAfterFirst = v1ModelsCatalog.__getCatalogBuilderRunsForTest(); assert.equal(runsAfterFirst, 1); - // Push the entry's age past CATALOG_STALE_WHILE_REVALIDATE_MS. - v1ModelsCatalog.__expireCatalogCacheForTest( - v1ModelsCatalog.CATALOG_STALE_WHILE_REVALIDATE_MS + 5_000 - ); + // Age well past the historical 30-second bound. Ordinary expiry must not turn a + // refresh failure into a client-visible cold-build wait. + v1ModelsCatalog.__expireCatalogCacheForTest(24 * 60 * 60 * 1000); const res2 = await v1ModelsCatalog.getUnifiedModelsResponse(makeRequest()); assert.equal(res2.status, 200); + assert.equal(await res2.text(), body1); assert.equal( v1ModelsCatalog.__getCatalogBuilderRunsForTest(), - runsAfterFirst + 1, - "past the staleness window, the builder must run again BEFORE the response is returned " + - "(a refresh that keeps failing must not pin a stale catalog forever)" + runsAfterFirst, + "ordinary TTL expiry must return the last successful snapshot before refreshing" ); + await v1ModelsCatalog.__flushCatalogBackgroundRefreshForTest(); + assert.equal(v1ModelsCatalog.__getCatalogBuilderRunsForTest(), runsAfterFirst + 1); }); test("5. a cached non-200 entry is never served as stale", async () => {