From 4c0e45d814d4192a1ef523cdcea32e42507a3af8 Mon Sep 17 00:00:00 2001 From: Dizzle <112548150+maxmad64bis@users.noreply.github.com> Date: Tue, 15 Sep 2026 16:21:58 +0200 Subject: [PATCH] fix(models): return 503 with Retry-After when a cold catalog build exceeds its time bound (#13438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cold `/v1/models` catalog build that exceeds its time bound now answers 503 with `Retry-After` instead of a 500, and the timed-out build stays joinable so the next retry does not start another cold build. Seven cases; the first fails on the tip (500 → 503). Validated first on the combined board of all 38 PRs of this batch (10 merged as-is, 28 after the maintainer rework) on top of release/v3.8.51 c0f92ec: typecheck:core, check:open-sse-typecheck and check:dashboard-typecheck clean; ESLint clean on every changed file; file-size (rebaselined for the combined growth), complexity, cognitive-complexity, changelog-integrity, docs-counts, docs-sync, migration-numbering and i18n new-key gates green; 735 focused node:test cases with the only batch-caused failure (a flag-count assertion) fixed. Then re-validated alone on the fresh release tip right before this merge: ESLint on the changed files, typecheck:core, check:open-sse-typecheck, the file-size/complexity/changelog gates and this PR's own tests. Thanks @maxmad64bis! --- changelog.d/fixes/13438-cold-catalog-retry.md | 1 + config/quality/eslint-suppressions.json | 5 - src/app/api/v1/models/catalogCache.ts | 79 +++++++++++-- .../12627-catalog-inflight-timeout.test.ts | 111 ++++++++++++++++-- 4 files changed, 172 insertions(+), 24 deletions(-) create mode 100644 changelog.d/fixes/13438-cold-catalog-retry.md diff --git a/changelog.d/fixes/13438-cold-catalog-retry.md b/changelog.d/fixes/13438-cold-catalog-retry.md new file mode 100644 index 0000000000..c03bb7ffb9 --- /dev/null +++ b/changelog.d/fixes/13438-cold-catalog-retry.md @@ -0,0 +1 @@ +- **fix(models):** return a retryable 503 with Retry-After instead of a 500 when the first catalog build outlasts its time bound ([#13438](https://github.com/diegosouzapw/OmniRoute/pull/13438)) — thanks @maxmad64bis diff --git a/config/quality/eslint-suppressions.json b/config/quality/eslint-suppressions.json index bd489ce752..2b2dabce66 100644 --- a/config/quality/eslint-suppressions.json +++ b/config/quality/eslint-suppressions.json @@ -1284,11 +1284,6 @@ "count": 10 } }, - "src/app/api/v1/models/catalogCache.ts": { - "@typescript-eslint/no-unused-vars": { - "count": 1 - } - }, "src/app/api/v1/models/catalogOpenrouter.ts": { "@typescript-eslint/no-unused-vars": { "count": 1 diff --git a/src/app/api/v1/models/catalogCache.ts b/src/app/api/v1/models/catalogCache.ts index aa585b86b1..ee0d504597 100644 --- a/src/app/api/v1/models/catalogCache.ts +++ b/src/app/api/v1/models/catalogCache.ts @@ -18,6 +18,7 @@ import { after } from "next/server"; import { getModelCatalogCacheVersion } from "@/lib/db/readCache"; import { extractApiKey } from "@/sse/services/auth"; +import { buildErrorBody } from "@omniroute/open-sse/utils/error"; import { catalogPageCacheKey, catalogStringResponse, parseCatalogPage } from "./catalogPagination"; import { isCodexModelCatalogClient } from "./catalogRequest"; @@ -148,9 +149,22 @@ function catalogBuildTimeoutMs(): number { const catalogLastGood = new Map(); +export class CatalogBuildTimeoutError extends Error { + constructor() { + super("catalog_build_timeout"); + this.name = "CatalogBuildTimeoutError"; + } +} + function withTimeout(promise: Promise, ms: number, label: string): Promise { return new Promise((resolve, reject) => { - const timer = setTimeout(() => reject(new Error(label)), ms); + const timer = setTimeout(() => { + if (label === "catalog_build_timeout") { + reject(new CatalogBuildTimeoutError()); + } else { + reject(new Error(label)); + } + }, ms); promise.then( (value) => { clearTimeout(timer); @@ -174,7 +188,12 @@ const catalogCache = new Map(); * It still resolves to its own original caller (that request legitimately waits * on it), just without being persisted. */ -type InFlightBuild = { generation: number; promise: Promise }; +type InFlightBuild = { + generation: number; + promise: Promise; + lastKeptAt?: number; + timeoutCount?: number; +}; const catalogInFlight = new Map(); let _catalogBuilderRuns = 0; @@ -250,8 +269,10 @@ function storePayload( }; if (buildGeneration === getModelCatalogCacheVersion()) { catalogCache.set(cacheKey, entry); + if (entry.status === 200) catalogLastGood.set(cacheKey, entry); } - if (entry.status === 200) catalogLastGood.set(cacheKey, entry); + // Cross-generation orphan: return entry to its original caller unchanged, + // persist neither cache nor lastGood. return entry; } @@ -302,7 +323,12 @@ function startBackgroundRefresh( // observes the failure. refreshPromise.catch(() => {}); - catalogInFlight.set(cacheKey, { generation, promise: refreshPromise }); + catalogInFlight.set(cacheKey, { + generation, + promise: refreshPromise, + lastKeptAt: Date.now(), + timeoutCount: 0, + }); refreshPromise .catch(() => {}) .finally(() => { @@ -329,12 +355,14 @@ async function awaitCatalogInFlight( try { payload = await withTimeout(inflight.promise, catalogBuildTimeoutMs(), "catalog_build_timeout"); } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - if (catalogInFlight.get(cacheKey)?.promise === inflight.promise) { - catalogInFlight.delete(cacheKey); + if (!(err instanceof CatalogBuildTimeoutError)) { + if (catalogInFlight.get(cacheKey)?.promise === inflight.promise) { + catalogInFlight.delete(cacheKey); + } + throw err; } const lastGood = catalogLastGood.get(cacheKey); - if (msg === "catalog_build_timeout" && lastGood) { + if (lastGood) { return catalogStringResponse( lastGood.body, mergeCatalogHeaders(corsHeaders, lastGood.headers, diagnosticHeaders, { @@ -343,7 +371,26 @@ async function awaitCatalogInFlight( lastGood.status ); } - throw err; + const shared = catalogInFlight.get(cacheKey); + if (shared && shared.promise === inflight.promise) { + shared.timeoutCount = (shared.timeoutCount ?? 0) + 1; + shared.lastKeptAt = Date.now(); + } + const boundMs = catalogBuildTimeoutMs(); + const retryAfterSec = Math.max(1, Math.ceil((2 * boundMs) / 1000)); + const body = JSON.stringify( + buildErrorBody(503, "catalog_build_timeout", undefined, { + type: "service_unavailable", + }) + ); + return catalogStringResponse( + body, + mergeCatalogHeaders(corsHeaders, diagnosticHeaders, { + "x-omniroute-catalog": "build-timeout", + "Retry-After": String(retryAfterSec), + }), + 503 + ); } return catalogStringResponse( payload.body, @@ -406,13 +453,21 @@ export async function resolveCachedCatalogResponse( // Only join an in-flight build from the CURRENT generation. A build bound to an // older (pre-write) generation reflects stale state, so a new request starts a // fresh build instead of joining it. - if (!inflight || inflight.generation !== currentGeneration) { + const boundMs = catalogBuildTimeoutMs(); + const existing = inflight; + const joinable = + !!existing && + existing.generation === currentGeneration && + Date.now() - (existing.lastKeptAt ?? 0) <= 3 * boundMs && + (existing.timeoutCount ?? 0) < 3; + if (!joinable) { const generation = currentGeneration; const promise = runBuilder(buildPayload, request).then((payload) => storePayload(cacheKey, payload, generation) ); - inflight = { generation, promise }; + inflight = { generation, promise, lastKeptAt: Date.now(), timeoutCount: 0 }; catalogInFlight.set(cacheKey, inflight); + promise.catch(() => {}); promise.finally(() => { if (catalogInFlight.get(cacheKey)?.promise === promise) catalogInFlight.delete(cacheKey); }); @@ -483,5 +538,7 @@ export function __forceCatalogInFlightRejectionForTest(request: Request, error: catalogInFlight.set(buildCatalogCacheKey(request), { generation: getModelCatalogCacheVersion(), promise: rejected, + lastKeptAt: Date.now(), + timeoutCount: 0, }); } diff --git a/tests/unit/12627-catalog-inflight-timeout.test.ts b/tests/unit/12627-catalog-inflight-timeout.test.ts index 62abeb4282..15bfeb9a6b 100644 --- a/tests/unit/12627-catalog-inflight-timeout.test.ts +++ b/tests/unit/12627-catalog-inflight-timeout.test.ts @@ -30,15 +30,18 @@ test.afterEach(() => { delete process.env.CATALOG_BUILD_TIMEOUT_MS; }); -test("#12627 cold hung rebuild times out instead of waiting forever", async () => { - await assert.rejects( - catalogCache.resolveCachedCatalogResponse( - request(), - { corsHeaders: {}, diagnosticHeaders: {} }, - neverResolves as (req: Request) => Promise - ), - /catalog_build_timeout/ +test("#12627 cold hung rebuild returns retryable 503", async () => { + const res = await catalogCache.resolveCachedCatalogResponse( + request(), + { corsHeaders: {}, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise ); + assert.equal(res.status, 503); + assert.equal(res.headers.get("Retry-After"), "1"); + assert.equal(res.headers.get("x-omniroute-catalog"), "build-timeout"); + const body = await res.json(); + assert.equal(body.error.message, "catalog_build_timeout"); + assert.equal(body.error.code, "service_unavailable"); }); test("#12627 timeout serves last-good 200 when a prior build succeeded", async () => { @@ -59,3 +62,95 @@ test("#12627 timeout serves last-good 200 when a prior build succeeded", async ( assert.equal(await second.text(), "good"); assert.equal(second.headers.get("x-omniroute-catalog"), "last-good"); }); + +test("cold real build error still rejects (never masked as 503)", async () => { + const err = new Error("boom"); + catalogCache.__forceCatalogInFlightRejectionForTest(request(), err); + await assert.rejects( + catalogCache.resolveCachedCatalogResponse( + request(), + { corsHeaders: {}, diagnosticHeaders: {} }, + async () => payload("unused") + ), + /boom/ + ); +}); + +test("rapid retry joins the orphaned build (one builder run, two 503s)", async () => { + const p1 = catalogCache.resolveCachedCatalogResponse( + request(), + { corsHeaders: {}, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise + ); + const p2 = catalogCache.resolveCachedCatalogResponse( + request(), + { corsHeaders: {}, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise + ); + const [r1, r2] = await Promise.all([p1, p2]); + assert.equal(r1.status, 503); + assert.equal(r2.status, 503); + assert.equal(catalogCache.__getCatalogBuilderRunsForTest(), 1); +}); + +test("diagnostic header merges without case duplicates", async () => { + const res = await catalogCache.resolveCachedCatalogResponse( + request(), + { corsHeaders: { "X-Omniroute-Catalog": "stale-value" }, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise + ); + assert.equal(res.status, 503); + const raw = [...res.headers.entries()].filter(([k]) => k.toLowerCase() === "x-omniroute-catalog"); + assert.equal(raw.length, 1); + assert.equal(raw[0][1], "build-timeout"); +}); + +test("slow build converging after two timeouts serves 200 on next retry", async () => { + const slowBuilder = async () => { + await new Promise((r) => setTimeout(r, 120)); + return payload("late-good"); + }; + const first = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, slowBuilder + ); + assert.equal(first.status, 503); + const second = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, slowBuilder + ); + assert.equal(second.status, 503); + await new Promise((r) => setTimeout(r, 200)); + const third = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, slowBuilder + ); + assert.equal(third.status, 200); + assert.equal(await third.text(), "late-good"); +}); + +test("eternally hung build is replaced, never pinned", async () => { + process.env.CATALOG_BUILD_TIMEOUT_MS = "20"; + try { + const r1 = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise + ); + assert.equal(r1.status, 503); + const r2 = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise + ); + assert.equal(r2.status, 503); + const r3 = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, + neverResolves as (req: Request) => Promise + ); + assert.equal(r3.status, 503); + const r4 = await catalogCache.resolveCachedCatalogResponse( + request(), { corsHeaders: {}, diagnosticHeaders: {} }, + async () => payload("fresh") + ); + assert.equal(catalogCache.__getCatalogBuilderRunsForTest() >= 2, true); + assert.equal([503, 200].includes(r4.status), true); + } finally { + process.env.CATALOG_BUILD_TIMEOUT_MS = "40"; + } +});