mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-15 19:32:20 +03:00
fix(models): return 503 with Retry-After when a cold catalog build exceeds its time bound (#13438)
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!
This commit is contained in:
1
changelog.d/fixes/13438-cold-catalog-retry.md
Normal file
1
changelog.d/fixes/13438-cold-catalog-retry.md
Normal file
@@ -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
|
||||
@@ -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
|
||||
|
||||
@@ -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<string, CachedCatalog>();
|
||||
|
||||
export class CatalogBuildTimeoutError extends Error {
|
||||
constructor() {
|
||||
super("catalog_build_timeout");
|
||||
this.name = "CatalogBuildTimeoutError";
|
||||
}
|
||||
}
|
||||
|
||||
function withTimeout<T>(promise: Promise<T>, ms: number, label: string): Promise<T> {
|
||||
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<string, CachedCatalog>();
|
||||
* It still resolves to its own original caller (that request legitimately waits
|
||||
* on it), just without being persisted.
|
||||
*/
|
||||
type InFlightBuild = { generation: number; promise: Promise<CachedCatalog> };
|
||||
type InFlightBuild = {
|
||||
generation: number;
|
||||
promise: Promise<CachedCatalog>;
|
||||
lastKeptAt?: number;
|
||||
timeoutCount?: number;
|
||||
};
|
||||
const catalogInFlight = new Map<string, InFlightBuild>();
|
||||
|
||||
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,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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<catalogCache.CatalogPayload>
|
||||
),
|
||||
/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<catalogCache.CatalogPayload>
|
||||
);
|
||||
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<catalogCache.CatalogPayload>
|
||||
);
|
||||
const p2 = catalogCache.resolveCachedCatalogResponse(
|
||||
request(),
|
||||
{ corsHeaders: {}, diagnosticHeaders: {} },
|
||||
neverResolves as (req: Request) => Promise<catalogCache.CatalogPayload>
|
||||
);
|
||||
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<catalogCache.CatalogPayload>
|
||||
);
|
||||
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<catalogCache.CatalogPayload>
|
||||
);
|
||||
assert.equal(r1.status, 503);
|
||||
const r2 = await catalogCache.resolveCachedCatalogResponse(
|
||||
request(), { corsHeaders: {}, diagnosticHeaders: {} },
|
||||
neverResolves as (req: Request) => Promise<catalogCache.CatalogPayload>
|
||||
);
|
||||
assert.equal(r2.status, 503);
|
||||
const r3 = await catalogCache.resolveCachedCatalogResponse(
|
||||
request(), { corsHeaders: {}, diagnosticHeaders: {} },
|
||||
neverResolves as (req: Request) => Promise<catalogCache.CatalogPayload>
|
||||
);
|
||||
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";
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user