diff --git a/changelog.d/fixes/11581-models-after-scheduler.md b/changelog.d/fixes/11581-models-after-scheduler.md new file mode 100644 index 00000000000..7a8133a0039 --- /dev/null +++ b/changelog.d/fixes/11581-models-after-scheduler.md @@ -0,0 +1 @@ +- **fix(api):** `/v1/models` stale-while-revalidate rebuilds now run in Next's `after()` instead of `setTimeout(…, 0)`, restoring the flush-before-rebuild guarantee from #8728. The route had been passing `{ scheduleBackgroundRefresh: (task) => after(task) }` as a third argument to `getUnifiedModelsResponse`, which has taken two parameters since #9199 removed the injection point — so the object was silently discarded and `catalogCache` never imported `after` at all. Because `setTimeout(…, 0)` only yields one turn of the event loop, a catalog rebuild that blocks for hundreds of milliseconds delayed the stale response it was supposed to run behind ([#11581](https://github.com/diegosouzapw/OmniRoute/pull/11581)) diff --git a/src/app/api/v1/models/catalog.ts b/src/app/api/v1/models/catalog.ts index 7a513943b66..04c67b35cb4 100644 --- a/src/app/api/v1/models/catalog.ts +++ b/src/app/api/v1/models/catalog.ts @@ -134,6 +134,7 @@ export { getCustomVisionCapabilityFields }; // 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 type { BackgroundRefreshScheduler } from "./catalogCache"; export { CATALOG_STALE_WHILE_REVALIDATE_MS, @@ -156,9 +157,19 @@ function yieldCatalogBuildTurn(): Promise { * Build unified OpenAI-compatible model catalog response. * Reused by `/api/v1/models` and `/api/v1` to avoid semantic drift (T09). */ +export type UnifiedModelsOptions = { + /** + * Where a stale-while-revalidate rebuild runs. Route handlers pass Next's + * `after()` so the rebuild starts only once the stale response has been + * flushed (#8728). Omitted, the cache falls back to `after()` itself. + */ + scheduleBackgroundRefresh?: BackgroundRefreshScheduler; +}; + export async function getUnifiedModelsResponse( request: Request, - corsHeaders: Record = {} + corsHeaders: Record = {}, + options?: UnifiedModelsOptions ) { const diagnosticHeaders = getCatalogDiagnosticsHeaders({ request }); @@ -200,6 +211,7 @@ export async function getUnifiedModelsResponse( hideAutoCombos: settingsForAuth?.hideAutoCombos === true || settingsForAuth?.autoRoutingEnabled === false, hideNoThinkVariants: settingsForAuth?.hideNoThinkVariants === true, + scheduleBackgroundRefresh: options?.scheduleBackgroundRefresh, } ); } catch (err) { diff --git a/src/app/api/v1/models/catalogCache.ts b/src/app/api/v1/models/catalogCache.ts index f2aa2973bda..5d6210c50f3 100644 --- a/src/app/api/v1/models/catalogCache.ts +++ b/src/app/api/v1/models/catalogCache.ts @@ -14,6 +14,8 @@ */ import { createHmac } from "node:crypto"; +import { after } from "next/server"; + import { getModelCatalogCacheVersion } from "@/lib/db/readCache"; import { extractApiKey } from "@/sse/services/auth"; @@ -192,26 +194,63 @@ function storePayload( * 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. */ +/** + * How a stale-while-revalidate rebuild gets off the request's critical path. + * + * Injectable because the correct answer differs by caller: a Next route handler + * must use `after()`, and a plain `http.Server` (or a test) has to use whatever + * it owns. The parameter is what #8728's response-flush guarantee is expressed + * in — see `resolveCachedCatalogResponse`. + */ +export type BackgroundRefreshScheduler = (task: () => void | Promise) => void; + +/** + * The scheduler used when a caller injects none. + * + * `after()` is what makes #8728's guarantee real: Next runs the callback once + * the response has been flushed to the client, so a rebuild that blocks the + * event loop for hundreds of milliseconds cannot delay the stale body the + * client was promised. `setTimeout(…, 0)` only yields a turn — it still runs + * while the response is in flight. + * + * `after()` throws outside a request scope, which is the normal case for unit + * tests and scripts that import this module directly. Falling back to a + * macrotask there keeps the old behaviour rather than failing the refresh: the + * flush ordering is meaningless when there is no response to flush. + */ +function defaultBackgroundRefreshScheduler(task: () => void | Promise): void { + try { + after(task); + } catch { + setTimeout(() => { + void task(); + }, 0); + } +} + function scheduleBackgroundRefresh( cacheKey: string, request: Request, - buildPayload: (request: Request) => Promise + buildPayload: (request: Request) => Promise, + schedule: BackgroundRefreshScheduler = defaultBackgroundRefreshScheduler ): void { if (catalogInFlight.has(cacheKey)) return; // a refresh for this key is already running const generation = getModelCatalogCacheVersion(); const refreshPromise: Promise = new Promise((resolve, reject) => { - setTimeout(() => { + schedule(() => runBuilder(buildPayload, request) - .then((payload) => resolve(storePayload(cacheKey, payload, generation))) + .then((payload) => { + resolve(storePayload(cacheKey, payload, generation)); + }) .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 @@ -242,11 +281,28 @@ function runBuilder( * 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. */ +export type CatalogResolveOptions = { + /** Cache-key dimensions. Only these two participate in the key. */ + hideAutoCombos?: boolean; + hideNoThinkVariants?: boolean; + /** + * Overrides `CATALOG_STALE_WHILE_REVALIDATE_MS` for this call. A function + * rather than a number so a caller can widen the window per request without + * this module caching a value that was only meant to hold once. + */ + getStaleWhileRevalidateMs?: () => number; + /** + * Where a stale-while-revalidate rebuild runs. Defaults to `after()`; a + * caller outside a Next request scope supplies its own. + */ + scheduleBackgroundRefresh?: BackgroundRefreshScheduler; +}; + export async function resolveCachedCatalogResponse( request: Request, headerSources: { corsHeaders: Record; diagnosticHeaders: Record }, buildPayload: (request: Request) => Promise, - catalogSettings?: { hideAutoCombos?: boolean; hideNoThinkVariants?: boolean } + catalogSettings?: CatalogResolveOptions ): Promise { const { corsHeaders, diagnosticHeaders } = headerSources; dropCatalogCacheIfStateChanged(); @@ -267,12 +323,15 @@ export async function resolveCachedCatalogResponse( // 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. - if ( - cached && - cached.status === 200 && - now - cached.expiresAt <= CATALOG_STALE_WHILE_REVALIDATE_MS - ) { - scheduleBackgroundRefresh(cacheKey, request, buildPayload); + const staleWindowMs = + catalogSettings?.getStaleWhileRevalidateMs?.() ?? CATALOG_STALE_WHILE_REVALIDATE_MS; + if (cached && cached.status === 200 && now - cached.expiresAt <= staleWindowMs) { + scheduleBackgroundRefresh( + cacheKey, + request, + buildPayload, + catalogSettings?.scheduleBackgroundRefresh + ); return new Response(cached.body, { status: cached.status, headers: mergeCatalogHeaders(corsHeaders, cached.headers, diagnosticHeaders),